From 37b85b6a399dba120de49d9056529852b2284793 Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Thu, 12 Oct 2017 18:30:52 -0400 Subject: [PATCH] Copy port[group] VIF info from extra to internal_info For API versions >= 1.28, Port & portgroup's .extra['vif_port_id'] was deprecated in Ocata. Before we can remove support for this, we need to copy that information to the object's internal_info['tenant_vif_port_id']. This copy/migration is done at the API layer when the user specifies the .extra[] value, as well as when the 'ironic db-sync online_data-migrations' is run. In order to know whether the ports and port groups have been migrated, their IronicObject versions are incremented. This also fixes it so that for API versions < 1.28, the deprecation warning is not shown, since we still need to support extra['vif_port_id'] in this case. When a port or portgroup's .extra['vif_port_id'] is removed via a PATCH API request, that VIF is removed from that object's internal_info. Change-Id: I69468c935e68dd9d37a474c318c3ceb9cdfc5868 Partial-Bug: 1722850 --- ironic/api/controllers/v1/port.py | 8 +- ironic/api/controllers/v1/portgroup.py | 9 +- ironic/api/controllers/v1/utils.py | 83 ++++++++ ironic/cmd/dbsync.py | 8 + ironic/common/release_mappings.py | 4 +- ironic/common/utils.py | 9 +- ironic/db/api.py | 10 + ironic/db/sqlalchemy/api.py | 23 +++ ironic/db/sqlalchemy/models.py | 17 ++ ironic/drivers/modules/network/common.py | 15 +- ironic/objects/port.py | 51 ++++- ironic/objects/portgroup.py | 68 ++++++- .../unit/api/controllers/v1/test_port.py | 185 +++++++++++++++++- .../unit/api/controllers/v1/test_portgroup.py | 165 +++++++++++++++- .../tests/unit/db/sqlalchemy/test_models.py | 30 +++ ironic/tests/unit/db/test_api.py | 41 ++++ .../drivers/modules/network/test_common.py | 82 +------- ironic/tests/unit/objects/test_objects.py | 4 +- ironic/tests/unit/objects/test_port.py | 75 ++++++- ironic/tests/unit/objects/test_portgroup.py | 83 ++++++++ .../migrate_vif_port_id-5e1496638240933d.yaml | 13 ++ 21 files changed, 857 insertions(+), 126 deletions(-) create mode 100644 ironic/tests/unit/db/sqlalchemy/test_models.py create mode 100644 releasenotes/notes/migrate_vif_port_id-5e1496638240933d.yaml diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index 0fbb14bd7d..6f8b274922 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -34,7 +34,6 @@ from ironic.api import expose from ironic.common import exception from ironic.common.i18n import _ from ironic.common import policy -from ironic.common import utils as common_utils from ironic import objects METRICS = metrics_utils.get_metrics_logger(__name__) @@ -563,10 +562,7 @@ class PortsController(rest.RestController): # request. raise exception.NotAcceptable() - extra = pdict.get('extra') - vif = extra.get('vif_port_id') if extra else None - if vif: - common_utils.warn_about_deprecated_extra_vif_port_id() + vif = api_utils.handle_post_port_like_extra_vif(pdict) if (pdict.get('portgroup_uuid') and (pdict.get('pxe_enabled') or vif)): @@ -654,6 +650,8 @@ class PortsController(rest.RestController): except api_utils.JSONPATCH_EXCEPTIONS as e: raise exception.PatchError(patch=patch, reason=e) + api_utils.handle_patch_port_like_extra_vif(rpc_port, port, patch) + if api_utils.is_path_removed(patch, '/portgroup_uuid'): rpc_port.portgroup_id = None diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 9ced692b99..09c4f409c8 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -30,7 +30,6 @@ from ironic.api import expose from ironic.common import exception from ironic.common.i18n import _ from ironic.common import policy -from ironic.common import utils as common_utils from ironic import objects METRICS = metrics_utils.get_metrics_logger(__name__) @@ -456,9 +455,8 @@ class PortgroupsController(pecan.rest.RestController): error_msg, status_code=http_client.BAD_REQUEST) pg_dict = portgroup.as_dict() - vif = pg_dict.get('extra', {}).get('vif_port_id') - if vif: - common_utils.warn_about_deprecated_extra_vif_port_id() + + api_utils.handle_post_port_like_extra_vif(pg_dict) # NOTE(yuriyz): UUID is mandatory for notifications payload if not pg_dict.get('uuid'): @@ -529,6 +527,9 @@ class PortgroupsController(pecan.rest.RestController): except api_utils.JSONPATCH_EXCEPTIONS as e: raise exception.PatchError(patch=patch, reason=e) + api_utils.handle_patch_port_like_extra_vif(rpc_portgroup, portgroup, + patch) + # Update only the fields that have changed for field in objects.Portgroup.fields: try: diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 2f07f8c135..6f6c268a5e 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -719,3 +719,86 @@ def allow_traits(): Version 1.37 of the API allows traits for the node. """ return pecan.request.version.minor >= versions.MINOR_37_NODE_TRAITS + + +def handle_post_port_like_extra_vif(p_dict): + """Handle a Post request that sets .extra['vif_port_id']. + + This handles attach of VIFs via specifying the VIF port ID + in a port or port group's extra['vif_port_id'] field. + + :param p_dict: a dictionary with field names/values for the port or + port group + :return: VIF or None + """ + vif = p_dict.get('extra', {}).get('vif_port_id') + if vif: + # TODO(rloo): in Stein cycle: if API version >= 1.28, remove + # warning and support for extra[]; else (< 1.28) + # still support it; continue copying to internal_info + # (see bug 1722850). i.e., change the 7 lines of code + # below to something like: + # if not api_utils.allow_vifs_subcontroller(): + # internal_info = {'tenant_vif_port_id': vif} + # pg_dict['internal_info'] = internal_info + if allow_vifs_subcontroller(): + utils.warn_about_deprecated_extra_vif_port_id() + # NOTE(rloo): this value should really be in .internal_info[..] + # which is what would happen if they had used the + # POST /v1/nodes//vifs API. + internal_info = {'tenant_vif_port_id': vif} + p_dict['internal_info'] = internal_info + return vif + + +def handle_patch_port_like_extra_vif(rpc_object, api_object, patch): + """Handle a Patch request that modifies .extra['vif_port_id']. + + This handles attach/detach of VIFs via the VIF port ID + in a port or port group's extra['vif_port_id'] field. + + :param rpc_object: a Port or Portgroup RPC object + :param api_object: the corresponding Port or Portgroup API object + :param patch: the JSON patch in the API request + """ + vif_list = get_patch_values(patch, '/extra/vif_port_id') + vif = None + if vif_list: + # if specified more than once, use the last value + vif = vif_list[-1] + + # TODO(rloo): in Stein cycle: if API version >= 1.28, remove this + # warning and don't copy to internal_info; else (<1.28) still + # support it; continue copying to internal_info (see bug 1722850). + # i.e., change the 8 lines of code below to something like: + # if not allow_vifs_subcontroller(): + # int_info = rpc_object.internal_info.get('tenant_vif_port_id') + # if (not int_info or + # int_info == rpc_object.extra.get('vif_port_id')): + # api_object.internal_info['tenant_vif_port_id'] = vif + if allow_vifs_subcontroller(): + utils.warn_about_deprecated_extra_vif_port_id() + # NOTE(rloo): if the user isn't also using the REST API + # 'POST nodes//vifs', we are safe to copy the + # .extra[] value to the .internal_info location + int_info = rpc_object.internal_info.get('tenant_vif_port_id') + if (not int_info or int_info == rpc_object.extra.get('vif_port_id')): + api_object.internal_info['tenant_vif_port_id'] = vif + + elif is_path_removed(patch, '/extra/vif_port_id'): + # TODO(rloo): in Stein cycle: if API version >= 1.28, remove this + # warning and don't remove from internal_info; else (<1.28) still + # support it; remove from internal_info (see bug 1722850). + # i.e., change the 8 lines of code below to something like: + # if not allow_vifs_subcontroller(): + # int_info = rpc_object.internal_info.get('tenant_vif...') + # if (int_info and int_info==rpc_object.extra.get('vif_port_id')): + # api_object.internal_info['tenant_vif_port_id'] = None + if allow_vifs_subcontroller(): + utils.warn_about_deprecated_extra_vif_port_id() + # NOTE(rloo): if the user isn't also using the REST API + # 'POST nodes//vifs', we are safe to remove the + # .extra[] value from the .internal_info location + int_info = rpc_object.internal_info.get('tenant_vif_port_id') + if (int_info and int_info == rpc_object.extra.get('vif_port_id')): + api_object.internal_info.pop('tenant_vif_port_id') diff --git a/ironic/cmd/dbsync.py b/ironic/cmd/dbsync.py index e8bf0259b8..2525b3d501 100644 --- a/ironic/cmd/dbsync.py +++ b/ironic/cmd/dbsync.py @@ -29,6 +29,8 @@ from ironic.common import service from ironic.conf import CONF from ironic.db import api as db_api from ironic.db import migration +from ironic.objects import port +from ironic.objects import portgroup from ironic import version @@ -64,6 +66,12 @@ dbapi = db_api.get_instance() ONLINE_MIGRATIONS = ( # TODO(dtantsur): remove when classic drivers are removed (Rocky?) (dbapi, 'migrate_to_hardware_types'), + # Added in Rocky + # TODO(rloo): remove in Stein + (port, 'migrate_vif_port_id'), + # Added in Rocky + # TODO(rloo): remove in Stein + (portgroup, 'migrate_vif_port_id'), ) diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 503a3b3d4f..3079b827b2 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -106,8 +106,8 @@ RELEASE_MAPPING = { 'Node': ['1.23'], 'Conductor': ['1.2'], 'Chassis': ['1.3'], - 'Port': ['1.7'], - 'Portgroup': ['1.3'], + 'Port': ['1.8'], + 'Portgroup': ['1.4'], 'Trait': ['1.0'], 'TraitList': ['1.0'], 'VolumeConnector': ['1.0'], diff --git a/ironic/common/utils.py b/ironic/common/utils.py index dd53dbfa1b..e69e0ef830 100644 --- a/ironic/common/utils.py +++ b/ironic/common/utils.py @@ -504,7 +504,8 @@ def warn_about_deprecated_extra_vif_port_id(): global warn_deprecated_extra_vif_port_id if not warn_deprecated_extra_vif_port_id: warn_deprecated_extra_vif_port_id = True - LOG.warning("Attaching VIF to a port/portgroup via " - "extra['vif_port_id'] is deprecated and will not " - "be supported in Pike release. API endpoint " - "v1/nodes//vifs should be used instead.") + LOG.warning("Starting with API version 1.28, attaching/detaching VIF " + "to/from a port or port group via extra['vif_port_id'] is " + "deprecated and will not be supported starting in the " + "Stein release. API endpoint v1/nodes//vifs should " + "be used instead.") diff --git a/ironic/db/api.py b/ironic/db/api.py index 1a70d8d822..273ef6059e 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -887,6 +887,16 @@ class Connection(object): ident does not exist. """ + @abc.abstractmethod + def get_not_versions(self, model_name, versions): + """Returns objects with versions that are not the specified versions. + + :param model_name: the name of the model (class) of desired objects + :param versions: list of versions of objects not to be returned + :returns: list of the DB objects + :raises: IronicException if there is no class associated with the name + """ + @abc.abstractmethod def check_versions(self): """Checks the whole database for incompatible objects. diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 72cfda8e00..7e6eecd272 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -1168,6 +1168,29 @@ class Connection(api.Connection): if count == 0: raise exception.VolumeTargetNotFound(target=ident) + def get_not_versions(self, model_name, versions): + """Returns objects with versions that are not the specified versions. + + This returns objects with versions that are not the specified versions. + Objects with null versions (there shouldn't be any) are also returned. + + :param model_name: the name of the model (class) of desired objects + :param versions: list of versions of objects not to be returned + :returns: list of the DB objects + :raises: IronicException if there is no class associated with the name + """ + if not versions: + return [] + + model = models.get_class(model_name) + + # NOTE(rloo): .notin_ does not handle null: + # http://docs.sqlalchemy.org/en/latest/core/sqlelement.html#sqlalchemy.sql.operators.ColumnOperators.notin_ + query = model_query(model).filter( + sql.or_(model.version == sql.null(), + model.version.notin_(versions))) + return query.all() + def check_versions(self): """Checks the whole database for incompatible objects. diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index c64f1a19d4..023e16edef 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/ironic/db/sqlalchemy/models.py @@ -30,6 +30,8 @@ from sqlalchemy import schema, String, Text from sqlalchemy.ext.declarative import declarative_base from sqlalchemy import orm +from ironic.common import exception +from ironic.common.i18n import _ from ironic.conf import CONF _DEFAULT_SQL_CONNECTION = 'sqlite:///' + path.join('$state_path', @@ -296,3 +298,18 @@ class NodeTrait(Base): primaryjoin='and_(NodeTrait.node_id == Node.id)', foreign_keys=node_id ) + + +def get_class(model_name): + """Returns the model class with the specified name. + + :param model_name: the name of the class + :returns: the class with the specified name + :raises: Exception if there is no class associated with the name + """ + for model in Base.__subclasses__(): + if model.__name__ == model_name: + return model + + raise exception.IronicException( + _("Cannot find model with name: %s") % model_name) diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index dd99da1411..9632df0445 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -26,7 +26,6 @@ from ironic.common import network from ironic.common import neutron from ironic.common.pxe_utils import DHCP_CLIENT_ID from ironic.common import states -from ironic.common import utils from ironic import objects CONF = cfg.CONF @@ -410,10 +409,7 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin): if 'extra' in port_obj.obj_what_changed(): original_port = objects.Port.get_by_id(context, port_obj.id) updated_client_id = port_obj.extra.get('client-id') - if (port_obj.extra.get('vif_port_id') and - (port_obj.extra['vif_port_id'] != - original_port.extra.get('vif_port_id'))): - utils.warn_about_deprecated_extra_vif_port_id() + if (original_port.extra.get('client-id') != updated_client_id): # DHCP Option with opt_value=None will remove it @@ -462,7 +458,6 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin): :raises: FailedToUpdateDHCPOptOnPort, Conflict """ - context = task.context portgroup_uuid = portgroup_obj.uuid # NOTE(vsaienko) address is not mandatory field in portgroup. # Do not touch neutron port if we removed address on portgroup. @@ -473,14 +468,6 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin): neutron.update_port_address(pg_vif, portgroup_obj.address, context=task.context) - if 'extra' in portgroup_obj.obj_what_changed(): - original_portgroup = objects.Portgroup.get_by_id(context, - portgroup_obj.id) - if (portgroup_obj.extra.get('vif_port_id') and - portgroup_obj.extra['vif_port_id'] != - original_portgroup.extra.get('vif_port_id')): - utils.warn_about_deprecated_extra_vif_port_id() - if ('standalone_ports_supported' in portgroup_obj.obj_what_changed()): if not portgroup_obj.standalone_ports_supported: diff --git a/ironic/objects/port.py b/ironic/objects/port.py index 0983887296..4aee841d2d 100644 --- a/ironic/objects/port.py +++ b/ironic/objects/port.py @@ -26,6 +26,36 @@ from ironic.objects import fields as object_fields from ironic.objects import notification +def migrate_vif_port_id(context, max_count): + """Copy port's VIF info from extra to internal_info. + + :param context: The context. + :param max_count: The maximum number of objects to migrate. Must be + >= 0. If zero, all the objects will be migrated. + :returns: A 2-tuple -- the total number of objects that need to be + migrated (at the beginning of this call) and the number + of migrated objects. + """ + # TODO(rloo): remove this method in Stein, when we remove from dbsync.py + + # NOTE(rloo): if we introduce newer port versions in the same cycle, + # we could add those versions along with 1.8. This is only so we don't + # duplicate work; it isn't necessary. + db_ports = Port.dbapi.get_not_versions('Port', ['1.8']) + total = len(db_ports) + max_count = max_count or total + done = 0 + for db_port in db_ports: + # NOTE(rloo): this will indirectly invoke Port._convert_to_version() + # which does all the real work + port = Port._from_db_object(context, Port(), db_port) + port.save() + done += 1 + if done == max_count: + break + return total, done + + @base.IronicObjectRegistry.register class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.0: Initial version @@ -38,7 +68,10 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # local_link_connection, portgroup_id and pxe_enabled # Version 1.6: Add internal_info field # Version 1.7: Add physical_network field - VERSION = '1.7' + # Version 1.8: Migrate/copy extra['vif_port_id'] to + # internal_info['tenant_vif_port_id'] (not an explicit db + # change) + VERSION = '1.8' dbapi = dbapi.get_instance() @@ -68,6 +101,11 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): None. For versions prior to this, it should be set to None (or removed). + Version 1.8: if extra['vif_port_id'] is specified (non-null) and + internal_info['tenant_vif_port_id'] is not specified, copy the + .extra value to internal_info. There is nothing to do here when + downgrading to an older version. + :param target_version: the desired version of the object :param remove_unavailable_fields: True to remove fields that are unavailable in the target version; set this to True when @@ -75,6 +113,17 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): values; set this to False for DB interactions. """ target_version = versionutils.convert_version_to_tuple(target_version) + if target_version >= (1, 8): + if self.obj_attr_is_set('extra'): + vif = self.extra.get('vif_port_id') + if vif: + internal_info = (self.internal_info + if self.obj_attr_is_set('internal_info') + else {}) + if 'tenant_vif_port_id' not in internal_info: + internal_info['tenant_vif_port_id'] = vif + self.internal_info = internal_info + # Convert the physical_network field. physnet_is_set = self.obj_attr_is_set('physical_network') if target_version >= (1, 7): diff --git a/ironic/objects/portgroup.py b/ironic/objects/portgroup.py index a12542d7a7..360f876f61 100644 --- a/ironic/objects/portgroup.py +++ b/ironic/objects/portgroup.py @@ -16,6 +16,7 @@ from oslo_utils import netutils from oslo_utils import strutils from oslo_utils import uuidutils +from oslo_utils import versionutils from oslo_versionedobjects import base as object_base from ironic.common import exception @@ -26,13 +27,47 @@ from ironic.objects import fields as object_fields from ironic.objects import notification +def migrate_vif_port_id(context, max_count): + """Copy portgroup's VIF info from extra to internal_info. + + :param context: The context. + :param max_count: The maximum number of objects to migrate. Must be + >= 0. If zero, all the objects will be migrated. + :returns: A 2-tuple -- the total number of objects that need to be + migrated (at the beginning of this call) and the number + of migrated objects. + """ + # TODO(rloo): remove this method in Stein, when we remove from dbsync.py + + # NOTE(rloo): if we introduce newer portgroup versions in the same cycle, + # we could add those versions along with 1.4. This is only so we don't + # duplicate work; it isn't necessary. + db_objs = Portgroup.dbapi.get_not_versions('Portgroup', ['1.4']) + total = len(db_objs) + max_count = max_count or total + done = 0 + for db_obj in db_objs: + # NOTE(rloo): this will indirectly invoke + # Portgroup._convert_to_version() which does all the real + # work + portgroup = Portgroup._from_db_object(context, Portgroup(), db_obj) + portgroup.save() + done += 1 + if done == max_count: + break + return total, done + + @base.IronicObjectRegistry.register class Portgroup(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.0: Initial version # Version 1.1: Add internal_info field # Version 1.2: Add standalone_ports_supported field # Version 1.3: Add mode and properties fields - VERSION = '1.3' + # Version 1.4: Migrate/copy extra['vif_port_id'] to + # internal_info['tenant_vif_port_id'] (not an explicit db + # change) + VERSION = '1.4' dbapi = dbapi.get_instance() @@ -49,6 +84,37 @@ class Portgroup(base.IronicObject, object_base.VersionedObjectDictCompat): 'properties': object_fields.FlexibleDictField(nullable=True), } + def _convert_to_version(self, target_version, + remove_unavailable_fields=True): + """Convert to the target version. + + Convert the object to the target version. The target version may be + the same, older, or newer than the version of the object. This is + used for DB interactions as well as for serialization/deserialization. + + Version 1.4: if extra['vif_port_id'] is specified (non-null) and + internal_info['tenant_vif_port_id'] is not specified, copy the + .extra value to internal_info. There is nothing to do here when + downgrading to an older version. + + :param target_version: the desired version of the object + :param remove_unavailable_fields: True to remove fields that are + unavailable in the target version; set this to True when + (de)serializing. False to set the unavailable fields to appropriate + values; set this to False for DB interactions. + """ + target_version = versionutils.convert_version_to_tuple(target_version) + if target_version >= (1, 4): + if self.obj_attr_is_set('extra'): + vif = self.extra.get('vif_port_id') + if vif: + internal_info = (self.internal_info + if self.obj_attr_is_set('internal_info') + else {}) + if 'tenant_vif_port_id' not in internal_info: + internal_info['tenant_vif_port_id'] = vif + self.internal_info = internal_info + # 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. diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py index bf9f720691..c627daea4b 100644 --- a/ironic/tests/unit/api/controllers/v1/test_port.py +++ b/ironic/tests/unit/api/controllers/v1/test_port.py @@ -1355,6 +1355,155 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual(http_client.FORBIDDEN, response.status_int) self.assertEqual('application/json', response.content_type) + def _test_add_extra_vif_port_id(self, port, headers, mock_warn, mock_upd): + response = self.patch_json( + '/ports/%s' % port.uuid, + [{'path': '/extra/vif_port_id', 'value': 'foo', 'op': 'add'}, + {'path': '/extra/vif_port_id', 'value': 'bar', 'op': 'add'}], + headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual({'vif_port_id': 'bar'}, + response.json['extra']) + self.assertTrue(mock_upd.called) + return response + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_add_extra_vif_port_id(self, mock_warn, mock_upd): + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31') + expected_intern_info = port.internal_info + expected_intern_info.update({'tenant_vif_port_id': 'bar'}) + headers = {api_base.Version.string: '1.27'} + response = self._test_add_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual(expected_intern_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_add_extra_vif_port_id_no_internal(self, mock_warn, mock_upd): + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31') + expected_intern_info = port.internal_info + expected_intern_info.update({'tenant_vif_port_id': 'bar'}) + headers = {api_base.Version.string: '1.27'} + response = self._test_add_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual(expected_intern_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_add_extra_vif_port_id_deprecated(self, mock_warn, mock_upd): + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31') + expected_intern_info = port.internal_info + expected_intern_info.update({'tenant_vif_port_id': 'bar'}) + headers = {api_base.Version.string: '1.34'} + response = self._test_add_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual(expected_intern_info, response.json['internal_info']) + self.assertTrue(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_replace_extra_vif_port_id(self, mock_warn, mock_upd): + extra = {'vif_port_id': 'original'} + internal_info = {'tenant_vif_port_id': 'original'} + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31', + extra=extra, + internal_info=internal_info) + expected_intern_info = port.internal_info + expected_intern_info.update({'tenant_vif_port_id': 'bar'}) + headers = {api_base.Version.string: '1.27'} + response = self._test_add_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual(expected_intern_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_add_extra_vif_port_id_diff_internal(self, mock_warn, mock_upd): + internal_info = {'tenant_vif_port_id': 'original'} + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31', + internal_info=internal_info) + headers = {api_base.Version.string: '1.27'} + response = self._test_add_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual(internal_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + def _test_remove_extra_vif_port_id(self, port, headers, mock_warn, + mock_upd): + response = self.patch_json( + '/ports/%s' % port.uuid, + [{'path': '/extra/vif_port_id', 'op': 'remove'}], + headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual({}, response.json['extra']) + self.assertTrue(mock_upd.called) + return response + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_remove_extra_vif_port_id(self, mock_warn, mock_upd): + internal_info = {'tenant_vif_port_id': 'bar'} + extra = {'vif_port_id': 'bar'} + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31', + internal_info=internal_info, + extra=extra) + headers = {api_base.Version.string: '1.27'} + response = self._test_remove_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual({}, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_remove_extra_vif_port_id_not_same(self, mock_warn, mock_upd): + # .internal_info['tenant_vif_port_id'] != .extra['vif_port_id'] + internal_info = {'tenant_vif_port_id': 'bar'} + extra = {'vif_port_id': 'foo'} + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31', + internal_info=internal_info, + extra=extra) + headers = {api_base.Version.string: '1.28'} + response = self._test_remove_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual(internal_info, response.json['internal_info']) + self.assertTrue(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_remove_extra_vif_port_id_not_internal(self, mock_warn, mock_upd): + # no .internal_info['tenant_vif_port_id'] + internal_info = {'foo': 'bar'} + extra = {'vif_port_id': 'bar'} + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:55:00:cf:2d:31', + internal_info=internal_info, + extra=extra) + headers = {api_base.Version.string: '1.28'} + response = self._test_remove_extra_vif_port_id(port, headers, + mock_warn, mock_upd) + self.assertEqual(internal_info, response.json['internal_info']) + self.assertTrue(mock_warn.called) + @mock.patch.object(rpcapi.ConductorAPI, 'create_port', autospec=True, side_effect=_rpcapi_create_port) @@ -1803,18 +1952,35 @@ class TestPost(test_api_base.BaseApiTest): self.assertEqual(http_client.FORBIDDEN, response.status_int) self.assertFalse(mock_create.called) + def _test_create_port_with_extra_vif_port_id(self, headers, mock_warn, + mock_create): + pdict = post_get_test_port(pxe_enabled=False, + extra={'vif_port_id': 'foo'}) + pdict.pop('physical_network') + response = self.post_json('/ports', pdict, headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.CREATED, response.status_int) + self.assertEqual({'vif_port_id': 'foo'}, response.json['extra']) + self.assertEqual({'tenant_vif_port_id': 'foo'}, + response.json['internal_info']) + mock_create.assert_called_once_with(mock.ANY, mock.ANY, mock.ANY, + 'test-topic') + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_create_port_with_extra_vif_port_id(self, mock_warn, mock_create): + headers = {api_base.Version.string: '1.27'} + self._test_create_port_with_extra_vif_port_id(headers, mock_warn, + mock_create) + self.assertFalse(mock_warn.called) + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', autospec=True) def test_create_port_with_extra_vif_port_id_deprecated(self, mock_warn, mock_create): - pdict = post_get_test_port(pxe_enabled=False, - extra={'vif_port_id': 'foo'}) - response = self.post_json('/ports', pdict, headers=self.headers) - self.assertEqual('application/json', response.content_type) - self.assertEqual(http_client.CREATED, response.status_int) - self.assertEqual(1, mock_warn.call_count) - mock_create.assert_called_once_with(mock.ANY, mock.ANY, mock.ANY, - 'test-topic') + self._test_create_port_with_extra_vif_port_id(self.headers, mock_warn, + mock_create) + self.assertTrue(mock_warn.called) def _test_create_port(self, mock_create, has_vif=False, in_portgroup=False, pxe_enabled=True, standalone_ports=True, @@ -1844,6 +2010,9 @@ class TestPost(test_api_base.BaseApiTest): self.assertEqual(expected_portgroup_uuid, response.json['portgroup_uuid']) self.assertEqual(extra, response.json['extra']) + if has_vif: + expected = {'tenant_vif_port_id': extra['vif_port_id']} + self.assertEqual(expected, response.json['internal_info']) mock_create.assert_called_once_with(mock.ANY, mock.ANY, mock.ANY, 'test-topic') else: diff --git a/ironic/tests/unit/api/controllers/v1/test_portgroup.py b/ironic/tests/unit/api/controllers/v1/test_portgroup.py index 7034acb304..431393810a 100644 --- a/ironic/tests/unit/api/controllers/v1/test_portgroup.py +++ b/ironic/tests/unit/api/controllers/v1/test_portgroup.py @@ -41,6 +41,16 @@ from ironic.tests.unit.api import utils as apiutils from ironic.tests.unit.objects import utils as obj_utils +def _rpcapi_update_portgroup(self, context, portgroup, topic): + """Fake used to mock out the conductor RPCAPI's update_portgroup method. + + Saves the updated portgroup object and returns the updated portgroup + as-per the real method. + """ + portgroup.save() + return portgroup + + class TestPortgroupObject(base.TestCase): def test_portgroup_init(self): @@ -965,6 +975,136 @@ class TestPatch(test_api_base.BaseApiTest): self.assertFalse(mock_upd.called) +@mock.patch.object(rpcapi.ConductorAPI, 'update_portgroup', autospec=True, + side_effect=_rpcapi_update_portgroup) +class TestPatchExtraVifPortId(test_api_base.BaseApiTest): + headers = {api_base.Version.string: str(api_v1.max_version())} + + def setUp(self): + super(TestPatchExtraVifPortId, self).setUp() + self.node = obj_utils.create_test_node(self.context) + self.portgroup = obj_utils.create_test_portgroup(self.context, + node_id=self.node.id) + p = mock.patch.object(rpcapi.ConductorAPI, 'get_topic_for') + self.mock_gtf = p.start() + self.mock_gtf.return_value = 'test-topic' + self.addCleanup(p.stop) + + def _test_add_extra_vif_port_id(self, headers, mock_warn, mock_upd): + extra = {'vif_port_id': 'bar'} + response = self.patch_json( + '/portgroups/%s' % self.portgroup.uuid, + [{'path': '/extra/vif_port_id', 'value': 'foo', 'op': 'add'}, + {'path': '/extra/vif_port_id', 'value': 'bar', 'op': 'add'}], + headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual(extra, response.json['extra']) + return response + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_add_extra_vif_port_id(self, mock_warn, mock_upd): + expected_intern_info = self.portgroup.internal_info + expected_intern_info.update({'tenant_vif_port_id': 'bar'}) + headers = {api_base.Version.string: '1.27'} + response = self._test_add_extra_vif_port_id(headers, mock_warn, + mock_upd) + self.assertEqual(expected_intern_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_add_extra_vif_port_id_deprecated(self, mock_warn, mock_upd): + expected_intern_info = self.portgroup.internal_info + expected_intern_info.update({'tenant_vif_port_id': 'bar'}) + response = self._test_add_extra_vif_port_id(self.headers, mock_warn, + mock_upd) + self.assertEqual(expected_intern_info, response.json['internal_info']) + self.assertTrue(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_replace_extra_vif_port_id(self, mock_warn, mock_upd): + self.portgroup.extra = {'vif_port_id': 'original'} + self.portgroup.internal_info = {'tenant_vif_port_id': 'original'} + self.portgroup.save() + expected_intern_info = self.portgroup.internal_info + expected_intern_info.update({'tenant_vif_port_id': 'bar'}) + headers = {api_base.Version.string: '1.27'} + response = self._test_add_extra_vif_port_id(headers, mock_warn, + mock_upd) + self.assertEqual(expected_intern_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_add_extra_vif_port_id_diff_internal(self, mock_warn, mock_upd): + internal_info = {'tenant_vif_port_id': 'original'} + self.portgroup.internal_info = internal_info + self.portgroup.save() + headers = {api_base.Version.string: '1.27'} + response = self._test_add_extra_vif_port_id(headers, mock_warn, + mock_upd) + # not changed + self.assertEqual(internal_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + def _test_remove_extra_vif_port_id(self, headers, mock_warn, mock_upd): + response = self.patch_json( + '/portgroups/%s' % self.portgroup.uuid, + [{'path': '/extra/vif_port_id', 'op': 'remove'}], + headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual({}, response.json['extra']) + self.assertTrue(mock_upd.called) + return response + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_remove_extra_vif_port_id(self, mock_warn, mock_upd): + self.portgroup.extra = {'vif_port_id': 'bar'} + orig_info = self.portgroup.internal_info.copy() + intern_info = self.portgroup.internal_info + intern_info.update({'tenant_vif_port_id': 'bar'}) + self.portgroup.internal_info = intern_info + self.portgroup.save() + headers = {api_base.Version.string: '1.27'} + response = self._test_remove_extra_vif_port_id(headers, mock_warn, + mock_upd) + self.assertEqual(orig_info, response.json['internal_info']) + self.assertFalse(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_remove_extra_vif_port_id_not_same(self, mock_warn, mock_upd): + # .internal_info['tenant_vif_port_id'] != .extra['vif_port_id'] + self.portgroup.extra = {'vif_port_id': 'foo'} + intern_info = self.portgroup.internal_info + intern_info.update({'tenant_vif_port_id': 'bar'}) + self.portgroup.internal_info = intern_info + self.portgroup.save() + headers = {api_base.Version.string: '1.28'} + response = self._test_remove_extra_vif_port_id(headers, mock_warn, + mock_upd) + self.assertEqual(intern_info, response.json['internal_info']) + self.assertTrue(mock_warn.called) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_remove_extra_vif_port_id_not_internal(self, mock_warn, mock_upd): + # no portgroup.internal_info['tenant_vif_port_id'] + self.portgroup.extra = {'vif_port_id': 'foo'} + self.portgroup.save() + intern_info = self.portgroup.internal_info + headers = {api_base.Version.string: '1.28'} + response = self._test_remove_extra_vif_port_id(headers, mock_warn, + mock_upd) + self.assertEqual(intern_info, response.json['internal_info']) + self.assertTrue(mock_warn.called) + + class TestPost(test_api_base.BaseApiTest): headers = {api_base.Version.string: str(api_v1.max_version())} @@ -1085,15 +1225,30 @@ class TestPost(test_api_base.BaseApiTest): headers=self.headers) self.assertEqual(pdict['extra'], result['extra']) + def _test_create_portgroup_with_extra_vif_port_id(self, headers, + mock_warn): + pgdict = apiutils.post_get_test_portgroup(extra={'vif_port_id': 'foo'}) + response = self.post_json('/portgroups', pgdict, headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.CREATED, response.status_int) + self.assertEqual({'vif_port_id': 'foo'}, response.json['extra']) + self.assertEqual({'tenant_vif_port_id': 'foo'}, + response.json['internal_info']) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_create_portgroup_with_extra_vif_port_id(self, mock_warn): + headers = {api_base.Version.string: '1.27'} + self._test_create_portgroup_with_extra_vif_port_id(headers, mock_warn) + self.assertFalse(mock_warn.called) + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', autospec=True) def test_create_portgroup_with_extra_vif_port_id_deprecated( self, mock_warn): - pgdict = apiutils.post_get_test_portgroup(extra={'vif_port_id': 'foo'}) - response = self.post_json('/portgroups', pgdict, headers=self.headers) - self.assertEqual('application/json', response.content_type) - self.assertEqual(http_client.CREATED, response.status_int) - self.assertEqual(1, mock_warn.call_count) + self._test_create_portgroup_with_extra_vif_port_id( + self.headers, mock_warn) + self.assertTrue(mock_warn.called) @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', autospec=True) diff --git a/ironic/tests/unit/db/sqlalchemy/test_models.py b/ironic/tests/unit/db/sqlalchemy/test_models.py new file mode 100644 index 0000000000..d7df66b4be --- /dev/null +++ b/ironic/tests/unit/db/sqlalchemy/test_models.py @@ -0,0 +1,30 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, 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 ironic.common import exception +from ironic.db.sqlalchemy import models +from ironic.tests import base as test_base + + +class TestGetClass(test_base.TestCase): + + def test_get_class(self): + ret = models.get_class('Chassis') + self.assertEqual(models.Chassis, ret) + + for model in models.Base.__subclasses__(): + ret = models.get_class(model.__name__) + self.assertEqual(model, ret) + + def test_get_class_bad(self): + self.assertRaises(exception.IronicException, + models.get_class, "DoNotExist") diff --git a/ironic/tests/unit/db/test_api.py b/ironic/tests/unit/db/test_api.py index 5f024dd3e8..104a7c1b3c 100644 --- a/ironic/tests/unit/db/test_api.py +++ b/ironic/tests/unit/db/test_api.py @@ -12,9 +12,11 @@ import mock from oslo_utils import uuidutils +from testtools import matchers from ironic.common import context from ironic.common import driver_factory +from ironic.common import exception from ironic.common import release_mappings from ironic.db import api as db_api from ironic.tests.unit.db import base @@ -97,3 +99,42 @@ class MigrateToHardwareTypesTestCase(base.DbTestCase): self.assertEqual((1, 1), result) node = self.dbapi.get_node_by_id(self.node.id) self.assertEqual('classic_driver', node.driver) + + +class GetNotVersionsTestCase(base.DbTestCase): + + def setUp(self): + super(GetNotVersionsTestCase, self).setUp() + self.dbapi = db_api.get_instance() + + def test_get_not_versions(self): + versions = ['1.1', '1.2', '1.3'] + node_uuids = [] + for v in versions: + node = utils.create_test_node(uuid=uuidutils.generate_uuid(), + version=v) + node_uuids.append(node.uuid) + self.assertEqual([], self.dbapi.get_not_versions('Node', versions)) + + res = self.dbapi.get_not_versions('Node', ['2.0']) + self.assertThat(res, matchers.HasLength(len(node_uuids))) + res_uuids = [n.uuid for n in res] + self.assertEqual(node_uuids, res_uuids) + + res = self.dbapi.get_not_versions('Node', versions[1:]) + self.assertThat(res, matchers.HasLength(1)) + self.assertEqual(node_uuids[0], res[0].uuid) + + def test_get_not_versions_null(self): + node = utils.create_test_node(uuid=uuidutils.generate_uuid(), + version=None) + node = self.dbapi.get_node_by_id(node.id) + self.assertIsNone(node.version) + res = self.dbapi.get_not_versions('Node', ['1.6']) + self.assertThat(res, matchers.HasLength(1)) + self.assertEqual(node.uuid, res[0].uuid) + + def test_get_not_versions_no_model(self): + utils.create_test_node(uuid=uuidutils.generate_uuid(), version='1.4') + self.assertRaises(exception.IronicException, + self.dbapi.get_not_versions, 'NotExist', ['1.6']) diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index a64fc348fe..c758bb8431 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -17,7 +17,6 @@ from oslo_utils import uuidutils from ironic.common import exception from ironic.common import neutron as neutron_common from ironic.common import states -from ironic.common import utils as common_utils from ironic.conductor import task_manager from ironic.drivers.modules.network import common from ironic.tests.unit.conductor import mgr_utils @@ -949,10 +948,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mock_get.assert_called_once_with(task, 'fake_vif_id') mock_clear.assert_called_once_with(self.port) - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) @mock.patch.object(neutron_common, 'update_port_address', autospec=True) - def test_port_changed_address(self, mac_update_mock, mock_warn): + def test_port_changed_address(self, mac_update_mock): new_address = '11:22:33:44:55:bb' self.port.address = new_address with task_manager.acquire(self.context, self.node.id) as task: @@ -960,7 +957,6 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mac_update_mock.assert_called_once_with( self.port.extra['vif_port_id'], new_address, context=task.context) - self.assertFalse(mock_warn.called) @mock.patch.object(neutron_common, 'update_port_address', autospec=True) def test_port_changed_address_VIF_MAC_update_fail(self, mac_update_mock): @@ -995,21 +991,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): dhcp_update_mock.assert_called_once_with( 'fake-id', expected_dhcp_opts, context=task.context) - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') - def test_port_changed_vif(self, dhcp_update_mock, mock_warn): - expected_extra = {'vif_port_id': 'new_ake-id', 'client-id': 'fake1'} - self.port.extra = expected_extra - with task_manager.acquire(self.context, self.node.id) as task: - self.interface.port_changed(task, self.port) - self.assertFalse(dhcp_update_mock.called) - self.assertEqual(1, mock_warn.call_count) - - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) - @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') - def test_port_changed_extra_add_new_key(self, dhcp_update_mock, mock_warn): + def test_port_changed_extra_add_new_key(self, dhcp_update_mock): self.port.extra = {'vif_port_id': 'fake-id'} self.port.save() expected_extra = self.port.extra @@ -1018,20 +1001,6 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.port_changed(task, self.port) self.assertFalse(dhcp_update_mock.called) - self.assertEqual(0, mock_warn.call_count) - - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) - @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') - def test_port_changed_extra_no_deprecation_if_removing_vif( - self, dhcp_update_mock, mock_warn): - self.port.extra = {'vif_port_id': 'foo'} - self.port.save() - self.port.extra = {'foo': 'bar'} - with task_manager.acquire(self.context, self.node.id) as task: - self.interface.port_changed(task, self.port) - self.assertFalse(dhcp_update_mock.called) - self.assertEqual(0, mock_warn.call_count) @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') def test_port_changed_client_id_fail(self, dhcp_update_mock): @@ -1225,48 +1194,6 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): self.interface.portgroup_changed(task, pg) self.assertFalse(mac_update_mock.called) - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) - @mock.patch.object(neutron_common, 'update_port_address', autospec=True) - def test_update_portgroup_vif(self, mac_update_mock, mock_warn): - pg = obj_utils.create_test_portgroup( - self.context, node_id=self.node.id) - extra = {'vif_port_id': 'foo'} - pg.extra = extra - with task_manager.acquire(self.context, self.node.id) as task: - self.interface.portgroup_changed(task, pg) - self.assertFalse(mac_update_mock.called) - self.assertEqual(1, mock_warn.call_count) - - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) - @mock.patch.object(neutron_common, 'update_port_address', autospec=True) - def test_update_portgroup_vif_removal_no_deprecation(self, mac_update_mock, - mock_warn): - pg = obj_utils.create_test_portgroup( - self.context, node_id=self.node.id, extra={'vif_port_id': 'foo'}) - pg.extra = {} - with task_manager.acquire(self.context, self.node.id) as task: - self.interface.portgroup_changed(task, pg) - self.assertFalse(mac_update_mock.called) - self.assertFalse(mock_warn.called) - - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) - @mock.patch.object(neutron_common, 'update_port_address', autospec=True) - def test_update_portgroup_extra_new_key(self, mac_update_mock, mock_warn): - pg = obj_utils.create_test_portgroup( - self.context, node_id=self.node.id, - extra={'vif_port_id': 'vif-id'}) - expected_extra = pg.extra - expected_extra['foo'] = 'bar' - pg.extra = expected_extra - with task_manager.acquire(self.context, self.node.id) as task: - self.interface.portgroup_changed(task, pg) - self.assertFalse(mac_update_mock.called) - self.assertFalse(mock_warn.called) - self.assertEqual(expected_extra, pg.extra) - @mock.patch.object(neutron_common, 'update_port_address', autospec=True) def test_update_portgroup_address_fail(self, mac_update_mock): pg = obj_utils.create_test_portgroup( @@ -1283,10 +1210,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): mac_update_mock.assert_called_once_with( 'fake-id', new_address, context=task.context) - @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', - autospec=True) @mock.patch.object(neutron_common, 'update_port_address', autospec=True) - def test_update_portgroup_address_no_vif(self, mac_update_mock, mock_warn): + def test_update_portgroup_address_no_vif(self, mac_update_mock): pg = obj_utils.create_test_portgroup( self.context, node_id=self.node.id) new_address = '11:22:33:44:55:bb' @@ -1295,7 +1220,6 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase): self.interface.portgroup_changed(task, pg) self.assertEqual(new_address, pg.address) self.assertFalse(mac_update_mock.called) - self.assertFalse(mock_warn.called) @mock.patch.object(neutron_common, 'update_port_address', autospec=True) def test_update_portgroup_nostandalone_ports_pxe_ports_exc( diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 762ddaa8ec..bbc4da236c 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -666,8 +666,8 @@ expected_object_fingerprints = { 'Node': '1.23-6bebf8dbcd2ce15407c946bd091f80b4', 'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', - 'Port': '1.7-898a47921f4a1f53fcdddd4eeb179e0b', - 'Portgroup': '1.3-71923a81a86743b313b190f5c675e258', + 'Port': '1.8-898a47921f4a1f53fcdddd4eeb179e0b', + 'Portgroup': '1.4-71923a81a86743b313b190f5c675e258', 'Conductor': '1.2-5091f249719d4a465062a1b3dc7f860d', 'EventType': '1.1-aa2ba1afd38553e3880c267404e8d370', 'NotificationPublisher': '1.0-51a09397d6c0687771fb5be9a999605d', diff --git a/ironic/tests/unit/objects/test_port.py b/ironic/tests/unit/objects/test_port.py index 734c0a9cf8..ac6a0bdf80 100644 --- a/ironic/tests/unit/objects/test_port.py +++ b/ironic/tests/unit/objects/test_port.py @@ -18,6 +18,7 @@ import types import mock from oslo_config import cfg +from oslo_utils import uuidutils from testtools import matchers from ironic.common import exception @@ -169,7 +170,9 @@ class TestConvertToVersion(db_base.DbTestCase): def setUp(self): super(TestConvertToVersion, self).setUp() - self.fake_port = db_utils.get_test_port() + self.vif_id = 'some_uuid' + extra = {'vif_port_id': self.vif_id} + self.fake_port = db_utils.get_test_port(extra=extra) def test_physnet_supported_missing(self): # Physical network not set, should be set to default. @@ -224,3 +227,73 @@ class TestConvertToVersion(db_base.DbTestCase): port._convert_to_version("1.6", False) self.assertIsNone(port.physical_network) self.assertEqual({}, port.obj_get_changes()) + + def test_vif_in_extra_lower_version(self): + # no conversion + port = objects.Port(self.context, **self.fake_port) + port._convert_to_version("1.7", False) + self.assertFalse('tenant_vif_port_id' in port.internal_info) + + def test_vif_in_extra(self): + for v in ['1.8', '1.9']: + port = objects.Port(self.context, **self.fake_port) + port._convert_to_version(v, False) + self.assertEqual(self.vif_id, + port.internal_info['tenant_vif_port_id']) + + def test_vif_in_extra_not_in_extra(self): + port = objects.Port(self.context, **self.fake_port) + port.extra.pop('vif_port_id') + port._convert_to_version('1.8', False) + self.assertFalse('tenant_vif_port_id' in port.internal_info) + + def test_vif_in_extra_in_internal_info(self): + vif2 = 'another_uuid' + port = objects.Port(self.context, **self.fake_port) + port.internal_info['tenant_vif_port_id'] = vif2 + port._convert_to_version('1.8', False) + # no change + self.assertEqual(vif2, port.internal_info['tenant_vif_port_id']) + + +class TestMigrateVifPortId(db_base.DbTestCase): + + def setUp(self): + super(TestMigrateVifPortId, self).setUp() + self.vif_id = 'some_uuid' + self.db_ports = [] + extra = {'vif_port_id': self.vif_id} + for i in range(3): + port = db_utils.create_test_port( + uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:3%s' % i, + extra=extra, version='1.7') + self.db_ports.append(port) + + @mock.patch.object(objects.Port, '_convert_to_version', autospec=True) + def test_migrate_vif_port_id_all(self, mock_convert): + with mock.patch.object(self.dbapi, 'get_not_versions', + autospec=True) as mock_get_not_versions: + mock_get_not_versions.return_value = self.db_ports + total, done = objects.port.migrate_vif_port_id(self.context, 0) + self.assertEqual(3, total) + self.assertEqual(3, done) + mock_get_not_versions.assert_called_once_with('Port', ['1.8']) + calls = 3 * [ + mock.call(mock.ANY, '1.8', remove_unavailable_fields=False), + ] + self.assertEqual(calls, mock_convert.call_args_list) + + @mock.patch.object(objects.Port, '_convert_to_version', autospec=True) + def test_migrate_vif_port_id_one(self, mock_convert): + with mock.patch.object(self.dbapi, 'get_not_versions', + autospec=True) as mock_get_not_versions: + mock_get_not_versions.return_value = self.db_ports + total, done = objects.port.migrate_vif_port_id(self.context, 1) + self.assertEqual(3, total) + self.assertEqual(1, done) + mock_get_not_versions.assert_called_once_with('Port', ['1.8']) + calls = [ + mock.call(mock.ANY, '1.8', remove_unavailable_fields=False), + ] + self.assertEqual(calls, mock_convert.call_args_list) diff --git a/ironic/tests/unit/objects/test_portgroup.py b/ironic/tests/unit/objects/test_portgroup.py index 74d4bcade8..ec67695c60 100644 --- a/ironic/tests/unit/objects/test_portgroup.py +++ b/ironic/tests/unit/objects/test_portgroup.py @@ -13,6 +13,7 @@ import datetime import mock +from oslo_utils import uuidutils from testtools import matchers from ironic.common import exception @@ -165,3 +166,85 @@ class TestPortgroupObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): def test_payload_schemas(self): self._check_payload_schemas(objects.portgroup, objects.Portgroup.fields) + + +class TestConvertToVersion(db_base.DbTestCase): + + def setUp(self): + super(TestConvertToVersion, self).setUp() + self.vif_id = 'some_uuid' + extra = {'vif_port_id': self.vif_id} + self.fake_portgroup = db_utils.get_test_portgroup(extra=extra) + + def test_vif_in_extra_lower_version(self): + # no conversion + portgroup = objects.Portgroup(self.context, **self.fake_portgroup) + portgroup._convert_to_version("1.3", False) + self.assertFalse('tenant_vif_port_id' in portgroup.internal_info) + + def test_vif_in_extra(self): + for v in ['1.4', '1.5']: + portgroup = objects.Portgroup(self.context, **self.fake_portgroup) + portgroup._convert_to_version(v, False) + self.assertEqual(self.vif_id, + portgroup.internal_info['tenant_vif_port_id']) + + def test_vif_in_extra_not_in_extra(self): + portgroup = objects.Portgroup(self.context, **self.fake_portgroup) + portgroup.extra.pop('vif_port_id') + portgroup._convert_to_version('1.4', False) + self.assertFalse('tenant_vif_port_id' in portgroup.internal_info) + + def test_vif_in_extra_in_internal_info(self): + vif2 = 'another_uuid' + portgroup = objects.Portgroup(self.context, **self.fake_portgroup) + portgroup.internal_info['tenant_vif_port_id'] = vif2 + portgroup._convert_to_version('1.4', False) + # no change + self.assertEqual(vif2, portgroup.internal_info['tenant_vif_port_id']) + + +class TestMigrateVifPortId(db_base.DbTestCase): + + def setUp(self): + super(TestMigrateVifPortId, self).setUp() + self.vif_id = 'some_uuid' + self.db_portgroups = [] + extra = {'vif_port_id': self.vif_id} + for i in range(3): + portgroup = db_utils.create_test_portgroup( + uuid=uuidutils.generate_uuid(), + name='pg%s' % i, + address='52:54:00:cf:2d:3%s' % i, + extra=extra, version='1.3') + self.db_portgroups.append(portgroup) + + @mock.patch.object(objects.Portgroup, '_convert_to_version', autospec=True) + def test_migrate_vif_port_id_all(self, mock_convert): + with mock.patch.object(self.dbapi, 'get_not_versions', + autospec=True) as mock_get_not_versions: + mock_get_not_versions.return_value = self.db_portgroups + total, done = objects.portgroup.migrate_vif_port_id( + self.context, 0) + self.assertEqual(3, total) + self.assertEqual(3, done) + mock_get_not_versions.assert_called_once_with('Portgroup', ['1.4']) + calls = 3 * [ + mock.call(mock.ANY, '1.4', remove_unavailable_fields=False), + ] + self.assertEqual(calls, mock_convert.call_args_list) + + @mock.patch.object(objects.Portgroup, '_convert_to_version', autospec=True) + def test_migrate_vif_port_id_one(self, mock_convert): + with mock.patch.object(self.dbapi, 'get_not_versions', + autospec=True) as mock_get_not_versions: + mock_get_not_versions.return_value = self.db_portgroups + total, done = objects.portgroup.migrate_vif_port_id( + self.context, 1) + self.assertEqual(3, total) + self.assertEqual(1, done) + mock_get_not_versions.assert_called_once_with('Portgroup', ['1.4']) + calls = [ + mock.call(mock.ANY, '1.4', remove_unavailable_fields=False), + ] + self.assertEqual(calls, mock_convert.call_args_list) diff --git a/releasenotes/notes/migrate_vif_port_id-5e1496638240933d.yaml b/releasenotes/notes/migrate_vif_port_id-5e1496638240933d.yaml new file mode 100644 index 0000000000..4f7d39354a --- /dev/null +++ b/releasenotes/notes/migrate_vif_port_id-5e1496638240933d.yaml @@ -0,0 +1,13 @@ +--- +upgrade: + - | + ``ironic-dbsync online_data_migrations`` will migrate any port's and + port group's extra['vif_port_id'] value to their + internal_info['tenant_vif_port_id']. + For API versions >= 1.28, the ability to attach/detach the VIF via + the port's or port group's extra['vif_port_id'] will not be supported + starting with the Stein release. + + Any out-of-tree network interface implementation that had a different + behavior in support of attach/detach VIFs via the port or port group's + extra['vif_port_id'] must be updated appropriately.