diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 916ddee96a..2ac6baabe0 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -28,6 +28,7 @@ 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__) @@ -446,8 +447,13 @@ class PortgroupsController(pecan.rest.RestController): raise wsme.exc.ClientSideError( 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() + new_portgroup = objects.Portgroup(pecan.request.context, - **portgroup.as_dict()) + **pg_dict) new_portgroup.create() # Set the HTTP Location Header pecan.response.location = link.build_url('portgroups', diff --git a/ironic/common/exception.py b/ironic/common/exception.py index b9612c25e9..1429fc01b5 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -228,7 +228,7 @@ class VolumeTargetBootIndexAlreadyExists(Conflict): class VifAlreadyAttached(Conflict): _msg_fmt = _("Unable to attach VIF because VIF %(vif)s is already " - "attached to Ironic port %(port_uuid)s") + "attached to Ironic %(object_type)s %(object_uuid)s") class NoFreePhysicalPorts(Invalid): diff --git a/ironic/common/utils.py b/ironic/common/utils.py index 8b221cc56b..30244e4e55 100644 --- a/ironic/common/utils.py +++ b/ironic/common/utils.py @@ -544,7 +544,7 @@ 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(_LW("Attaching VIF to a port via " + LOG.warning(_LW("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.")) diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index 150f9b4921..800e8a7994 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + from neutronclient.common import exceptions as neutron_exceptions from oslo_config import cfg from oslo_log import log @@ -30,6 +32,108 @@ LOG = log.getLogger(__name__) TENANT_VIF_KEY = 'tenant_vif_port_id' +def _vif_attached(port_like_obj, vif_id): + """Check if VIF is already attached to a port or portgroup. + + Raises an exception if a VIF with id=vif_id is attached to the port-like + (Port or Portgroup) object. Otherwise, returns whether a VIF is attached. + + :param port_like_obj: port-like object to check. + :param vif_id: identifier of the VIF to look for in port_like_obj. + :returns: True if a VIF (but not vif_id) is attached to port_like_obj, + False otherwise. + :raises: VifAlreadyAttached, if vif_id is attached to port_like_obj. + """ + attached_vif_id = port_like_obj.internal_info.get( + TENANT_VIF_KEY, port_like_obj.extra.get('vif_port_id')) + if attached_vif_id == vif_id: + raise exception.VifAlreadyAttached( + object_type=port_like_obj.__class__.__name__, + vif=vif_id, object_uuid=port_like_obj.uuid) + return attached_vif_id is not None + + +def _get_free_portgroups_and_ports(task, vif_id): + """Get free portgroups and ports. + + It only returns ports or portgroups that can be used for attachment of + vif_id. + + :param task: a TaskManager instance. + :param vif_id: Name or UUID of a VIF. + :returns: tuple of: list of free portgroups, list of free ports. + :raises: VifAlreadyAttached, if vif_id is attached to any of the + node's ports or portgroups. + """ + + # This list contains ports selected as candidates for attachment + free_ports = [] + # This is a mapping of portgroup id to collection of its free ports + ports_by_portgroup = collections.defaultdict(list) + # This set contains IDs of portgroups that should be ignored, as they have + # at least one port with vif already attached to it + non_usable_portgroups = set() + + for p in task.ports: + if _vif_attached(p, vif_id): + # Consider such portgroup unusable. The fact that we can have None + # added in this set is not a problem + non_usable_portgroups.add(p.portgroup_id) + continue + if p.portgroup_id is None: + # ports without portgroup_id are always considered candidates + free_ports.append(p) + else: + ports_by_portgroup[p.portgroup_id].append(p) + + # This list contains portgroups selected as candidates for attachment + free_portgroups = [] + + for pg in task.portgroups: + if _vif_attached(pg, vif_id): + continue + if pg.id in non_usable_portgroups: + # This portgroup has vifs attached to its ports, consider its + # ports instead to avoid collisions + free_ports.extend(ports_by_portgroup[pg.id]) + # Also ignore empty portgroups + elif ports_by_portgroup[pg.id]: + free_portgroups.append(pg) + + return free_portgroups, free_ports + + +def get_free_port_like_object(task, vif_id): + """Find free port-like object (portgroup or port) VIF will be attached to. + + Ensures that VIF is not already attached to this node. It will return the + first free port group. If there are no free port groups, then the first + available port (pxe_enabled preferably) is used. + + :param task: a TaskManager instance. + :param vif_id: Name or UUID of a VIF. + :raises: VifAlreadyAttached, if VIF is already attached to the node. + :raises: NoFreePhysicalPorts, if there is no port-like object VIF can be + attached to. + :returns: port-like object VIF will be attached to. + """ + + free_portgroups, free_ports = _get_free_portgroups_and_ports(task, vif_id) + + if free_portgroups: + # portgroups are higher priority + return free_portgroups[0] + + if not free_ports: + raise exception.NoFreePhysicalPorts(vif=vif_id) + + # Sort ports by pxe_enabled to ensure we always bind pxe_enabled ports + # first + sorted_free_ports = sorted(free_ports, key=lambda p: p.pxe_enabled, + reverse=True) + return sorted_free_ports[0] + + class VIFPortIDMixin(object): def port_changed(self, task, port_obj): @@ -112,13 +216,25 @@ class VIFPortIDMixin(object): context = task.context portgroup_uuid = portgroup_obj.uuid - if 'address' in portgroup_obj.obj_what_changed(): - pg_vif = portgroup_obj.extra.get('vif_port_id') + # NOTE(vsaienko) address is not mandatory field in portgroup. + # Do not touch neutron port if we removed address on portgroup. + if ('address' in portgroup_obj.obj_what_changed() and + portgroup_obj.address): + pg_vif = (portgroup_obj.internal_info.get(TENANT_VIF_KEY) or + portgroup_obj.extra.get('vif_port_id')) if pg_vif: neutron.update_port_address(pg_vif, portgroup_obj.address, token=context.auth_token) + 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: @@ -149,9 +265,9 @@ class VIFPortIDMixin(object): entry with the ID of the VIF. """ vifs = [] - for port in task.ports: - vif_id = port.internal_info.get( - TENANT_VIF_KEY, port.extra.get('vif_port_id')) + for port_like_obj in task.ports + task.portgroups: + vif_id = port_like_obj.internal_info.get( + TENANT_VIF_KEY, port_like_obj.extra.get('vif_port_id')) if vif_id: vifs.append({'id': vif_id}) return vifs @@ -159,6 +275,10 @@ class VIFPortIDMixin(object): def vif_attach(self, task, vif_info): """Attach a virtual network interface to a node + Attach a virtual interface to a node. It will use the first free port + group. If there are no free port groups, then the first available port + (pxe_enabled preferably) is used. + :param task: A TaskManager instance. :param vif_info: a dictionary of information about a VIF. It must have an 'id' key, whose value is a unique @@ -166,53 +286,39 @@ class VIFPortIDMixin(object): :raises: NetworkError, VifAlreadyAttached, NoFreePhysicalPorts """ vif_id = vif_info['id'] - # Sort ports by pxe_enabled to ensure we always bind pxe_enabled ports - # first - sorted_ports = sorted(task.ports, key=lambda p: p.pxe_enabled, - reverse=True) - free_ports = [] - # Check all ports to ensure this VIF isn't already attached - for port in sorted_ports: - port_id = port.internal_info.get(TENANT_VIF_KEY, - port.extra.get('vif_port_id')) - if port_id is None: - free_ports.append(port) - elif port_id == vif_id: - raise exception.VifAlreadyAttached( - vif=vif_id, port_uuid=port.uuid) - if not free_ports: - raise exception.NoFreePhysicalPorts(vif=vif_id) + port_like_obj = get_free_port_like_object(task, vif_id) - # Get first free port - port = free_ports.pop(0) - - # Check if the requested vif_id is a neutron port. If it is - # then attempt to update the port's MAC address. - try: - client = neutron.get_client(task.context.auth_token) - client.show_port(vif_id) - except neutron_exceptions.NeutronClientException: - # NOTE(sambetts): If a client error occurs this is because either - # neutron doesn't exist because we're running in standalone - # environment or we can't find a matching neutron port which means - # a user might be requesting a non-neutron port. So skip trying to - # update the neutron port MAC address in these cases. - pass - else: + # Address is optional for portgroups + if port_like_obj.address: + # Check if the requested vif_id is a neutron port. If it is + # then attempt to update the port's MAC address. try: - neutron.update_port_address(vif_id, port.address) - except exception.FailedToUpdateMacOnPort: - raise exception.NetworkError(_( - "Unable to attach VIF %(vif)s because Ironic can not " - "update Neutron port %(port)s MAC address to match " - "physical MAC address %(mac)s") % { - 'vif': vif_id, 'port': vif_id, 'mac': port.address}) + client = neutron.get_client(task.context.auth_token) + client.show_port(vif_id) + except neutron_exceptions.NeutronClientException: + # NOTE(sambetts): If a client error occurs this is because + # either neutron doesn't exist because we're running in + # standalone environment or we can't find a matching neutron + # port which means a user might be requesting a non-neutron + # port. So skip trying to update the neutron port MAC address + # in these cases. + pass + else: + try: + neutron.update_port_address(vif_id, port_like_obj.address) + except exception.FailedToUpdateMacOnPort: + raise exception.NetworkError(_( + "Unable to attach VIF %(vif)s because Ironic can not " + "update Neutron port %(port)s MAC address to match " + "physical MAC address %(mac)s") % { + 'vif': vif_id, 'port': vif_id, + 'mac': port_like_obj.address}) - int_info = port.internal_info + int_info = port_like_obj.internal_info int_info[TENANT_VIF_KEY] = vif_id - port.internal_info = int_info - port.save() + port_like_obj.internal_info = int_info + port_like_obj.save() def vif_detach(self, task, vif_id): """Detach a virtual network interface from a node @@ -221,18 +327,21 @@ class VIFPortIDMixin(object): :param vif_id: A VIF ID to detach :raises: VifNotAttached """ - for port in task.ports: + + ports = [p for p in task.ports if p.portgroup_id is None] + portgroups = task.portgroups + for port_like_obj in portgroups + ports: # FIXME(sambetts) Remove this when we no longer support a nova # driver that uses port.extra - if (port.extra.get("vif_port_id") == vif_id or - port.internal_info.get(TENANT_VIF_KEY) == vif_id): - int_info = port.internal_info - extra = port.extra + if (port_like_obj.extra.get("vif_port_id") == vif_id or + port_like_obj.internal_info.get(TENANT_VIF_KEY) == vif_id): + int_info = port_like_obj.internal_info + extra = port_like_obj.extra int_info.pop(TENANT_VIF_KEY, None) extra.pop('vif_port_id', None) - port.extra = extra - port.internal_info = int_info - port.save() + port_like_obj.extra = extra + port_like_obj.internal_info = int_info + port_like_obj.save() break else: raise exception.VifNotAttached(vif=vif_id, node=task.node.uuid) @@ -252,5 +361,5 @@ class VIFPortIDMixin(object): return (p_obj.internal_info.get('cleaning_vif_port_id') or p_obj.internal_info.get('provisioning_vif_port_id') or - p_obj.internal_info.get('tenant_vif_port_id') or + p_obj.internal_info.get(TENANT_VIF_KEY) or p_obj.extra.get('vif_port_id') or None) diff --git a/ironic/drivers/modules/network/flat.py b/ironic/drivers/modules/network/flat.py index dda7b584c5..aadc9614d5 100644 --- a/ironic/drivers/modules/network/flat.py +++ b/ironic/drivers/modules/network/flat.py @@ -69,17 +69,12 @@ class FlatNetwork(common.VIFPortIDMixin, neutron.NeutronNetworkInterfaceMixin, if not host_id: return - # FIXME(sambetts): Uncomment when we support vifs attached to - # portgroups - # - # ports = [p for p in task.ports if not p.portgroup_id] - # portgroups = task.portgroups - client = neutron.get_client(task.context.auth_token) - for port_like_obj in task.ports: # + portgroups: - vif_port_id = (port_like_obj.extra.get('vif_port_id') or - port_like_obj.internal_info.get( - 'tenant_vif_port_id')) + for port_like_obj in task.ports + task.portgroups: + vif_port_id = ( + port_like_obj.internal_info.get('tenant_vif_port_id') or + port_like_obj.extra.get('vif_port_id') + ) if not vif_port_id: continue body = { diff --git a/ironic/tests/unit/api/v1/test_portgroups.py b/ironic/tests/unit/api/v1/test_portgroups.py index b80a002297..45920f10e9 100644 --- a/ironic/tests/unit/api/v1/test_portgroups.py +++ b/ironic/tests/unit/api/v1/test_portgroups.py @@ -30,6 +30,7 @@ from ironic.api.controllers import v1 as api_v1 from ironic.api.controllers.v1 import portgroup as api_portgroup from ironic.api.controllers.v1 import utils as api_utils from ironic.common import exception +from ironic.common import utils as common_utils from ironic.conductor import rpcapi from ironic.tests import base from ironic.tests.unit.api import base as test_api_base @@ -875,8 +876,10 @@ class TestPost(test_api_base.BaseApiTest): super(TestPost, self).setUp() self.node = obj_utils.create_test_node(self.context) + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) @mock.patch.object(timeutils, 'utcnow', autospec=True) - def test_create_portgroup(self, mock_utcnow): + def test_create_portgroup(self, mock_utcnow, mock_warn): pdict = apiutils.post_get_test_portgroup() test_time = datetime.datetime(2000, 1, 1, 0, 0) mock_utcnow.return_value = test_time @@ -895,6 +898,7 @@ class TestPost(test_api_base.BaseApiTest): expected_location = '/v1/portgroups/%s' % pdict['uuid'] self.assertEqual(urlparse.urlparse(response.location).path, expected_location) + self.assertEqual(0, mock_warn.call_count) @mock.patch.object(timeutils, 'utcnow', autospec=True) def test_create_portgroup_v123(self, mock_utcnow): @@ -956,6 +960,26 @@ class TestPost(test_api_base.BaseApiTest): headers=self.headers) self.assertEqual(pdict['extra'], result['extra']) + @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) + + @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', + autospec=True) + def test_create_portgroup_with_no_extra(self, mock_warn): + pgdict = apiutils.post_get_test_portgroup() + del pgdict['extra'] + 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(0, mock_warn.call_count) + def test_create_portgroup_no_address(self): pdict = apiutils.post_get_test_portgroup() del pdict['address'] diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index f1ab657ca0..5064c00af8 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -26,6 +26,165 @@ from ironic.tests.unit.objects import utils as obj_utils CONF = cfg.CONF +class TestCommonFunctions(db_base.DbTestCase): + + def setUp(self): + super(TestCommonFunctions, self).setUp() + self.config(enabled_drivers=['fake']) + mgr_utils.mock_the_extension_manager() + self.node = obj_utils.create_test_node(self.context, + network_interface='neutron') + self.port = obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:32') + self.vif_id = "fake_vif_id" + + def _objects_setup(self): + pg1 = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id) + pg1_ports = [] + # This portgroup contains 2 ports, both of them without VIF + for i in range(2): + pg1_ports.append(obj_utils.create_test_port( + self.context, node_id=self.node.id, + address='52:54:00:cf:2d:0%d' % i, + uuid=uuidutils.generate_uuid(), portgroup_id=pg1.id)) + pg2 = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, address='00:54:00:cf:2d:04', + name='foo2', uuid=uuidutils.generate_uuid()) + pg2_ports = [] + # This portgroup contains 3 ports, one of them with 'some-vif' + # attached, so the two free ones should be considered standalone + for i in range(2, 4): + pg2_ports.append(obj_utils.create_test_port( + self.context, node_id=self.node.id, + address='52:54:00:cf:2d:0%d' % i, + uuid=uuidutils.generate_uuid(), portgroup_id=pg2.id)) + pg2_ports.append(obj_utils.create_test_port( + self.context, node_id=self.node.id, + address='52:54:00:cf:2d:04', + extra={'vif_port_id': 'some-vif'}, + uuid=uuidutils.generate_uuid(), portgroup_id=pg2.id)) + # This portgroup has 'some-vif-2' attached to it and contains one port, + # so neither portgroup nor port can be considered free + pg3 = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, address='00:54:00:cf:2d:05', + name='foo3', uuid=uuidutils.generate_uuid(), + internal_info={common.TENANT_VIF_KEY: 'some-vif-2'}) + pg3_ports = [obj_utils.create_test_port( + self.context, node_id=self.node.id, + address='52:54:00:cf:2d:05', uuid=uuidutils.generate_uuid(), + portgroup_id=pg3.id)] + return pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports + + def test__get_free_portgroups_and_ports(self): + pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports = self._objects_setup() + with task_manager.acquire(self.context, self.node.id) as task: + free_portgroups, free_ports = ( + common._get_free_portgroups_and_ports(task, self.vif_id)) + self.assertItemsEqual( + [self.port.uuid] + [p.uuid for p in pg2_ports[:2]], + [p.uuid for p in free_ports]) + self.assertItemsEqual([pg1.uuid], [p.uuid for p in free_portgroups]) + + def test_get_free_port_like_object_ports(self): + with task_manager.acquire(self.context, self.node.id) as task: + res = common.get_free_port_like_object(task, self.vif_id) + self.assertEqual(self.port.uuid, res.uuid) + + def test_get_free_port_like_object_ports_pxe_enabled_first(self): + self.port.pxe_enabled = False + self.port.save() + other_port = obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:33', + uuid=uuidutils.generate_uuid()) + with task_manager.acquire(self.context, self.node.id) as task: + res = common.get_free_port_like_object(task, self.vif_id) + self.assertEqual(other_port.uuid, res.uuid) + + def test_get_free_port_like_object_portgroup_first(self): + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id) + obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:01', + uuid=uuidutils.generate_uuid(), portgroup_id=pg.id) + with task_manager.acquire(self.context, self.node.id) as task: + res = common.get_free_port_like_object(task, self.vif_id) + self.assertEqual(pg.uuid, res.uuid) + + def test_get_free_port_like_object_ignores_empty_portgroup(self): + obj_utils.create_test_portgroup(self.context, node_id=self.node.id) + with task_manager.acquire(self.context, self.node.id) as task: + res = common.get_free_port_like_object(task, self.vif_id) + self.assertEqual(self.port.uuid, res.uuid) + + def test_get_free_port_like_object_ignores_standalone_portgroup(self): + self.port.destroy() + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id) + obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:01', + uuid=uuidutils.generate_uuid(), portgroup_id=pg.id, + extra={'vif_port_id': 'some-vif'}) + free_port = obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:02', + uuid=uuidutils.generate_uuid(), portgroup_id=pg.id) + with task_manager.acquire(self.context, self.node.id) as task: + res = common.get_free_port_like_object(task, self.vif_id) + self.assertEqual(free_port.uuid, res.uuid) + + def test_get_free_port_like_object_vif_attached_to_portgroup(self): + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, + internal_info={common.TENANT_VIF_KEY: self.vif_id}) + obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:01', + uuid=uuidutils.generate_uuid(), portgroup_id=pg.id) + with task_manager.acquire(self.context, self.node.id) as task: + self.assertRaisesRegex( + exception.VifAlreadyAttached, + r"already attached to Ironic Portgroup", + common.get_free_port_like_object, task, self.vif_id) + + def test_get_free_port_like_object_vif_attached_to_portgroup_extra(self): + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, + extra={'vif_port_id': self.vif_id}) + obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:01', + uuid=uuidutils.generate_uuid(), portgroup_id=pg.id) + with task_manager.acquire(self.context, self.node.id) as task: + self.assertRaisesRegex( + exception.VifAlreadyAttached, + r"already attached to Ironic Portgroup", + common.get_free_port_like_object, task, self.vif_id) + + def test_get_free_port_like_object_vif_attached_to_port(self): + self.port.internal_info = {common.TENANT_VIF_KEY: self.vif_id} + self.port.save() + with task_manager.acquire(self.context, self.node.id) as task: + self.assertRaisesRegex( + exception.VifAlreadyAttached, + r"already attached to Ironic Port\b", + common.get_free_port_like_object, task, self.vif_id) + + def test_get_free_port_like_object_vif_attached_to_port_extra(self): + self.port.extra = {'vif_port_id': self.vif_id} + self.port.save() + with task_manager.acquire(self.context, self.node.id) as task: + self.assertRaisesRegex( + exception.VifAlreadyAttached, + r"already attached to Ironic Port\b", + common.get_free_port_like_object, task, self.vif_id) + + def test_get_free_port_like_object_nothing_free(self): + self.port.extra = {'vif_port_id': 'another-vif'} + self.port.save() + with task_manager.acquire(self.context, self.node.id) as task: + self.assertRaises(exception.NoFreePhysicalPorts, + common.get_free_port_like_object, + task, self.vif_id) + + class TestVifPortIDMixin(db_base.DbTestCase): def setUp(self): @@ -47,17 +206,33 @@ class TestVifPortIDMixin(db_base.DbTestCase): vif_id = uuidutils.generate_uuid() self.port.extra = {'vif_port_id': vif_id} self.port.save() + pg_vif_id = uuidutils.generate_uuid() + portgroup = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, + address='52:54:00:00:00:00', + internal_info={common.TENANT_VIF_KEY: pg_vif_id}) + obj_utils.create_test_port( + self.context, node_id=self.node.id, portgroup_id=portgroup.id, + address='52:54:00:cf:2d:01', uuid=uuidutils.generate_uuid()) with task_manager.acquire(self.context, self.node.id) as task: vifs = self.interface.vif_list(task) - self.assertEqual([{'id': vif_id}], vifs) + self.assertItemsEqual([{'id': pg_vif_id}, {'id': vif_id}], vifs) def test_vif_list_internal(self): vif_id = uuidutils.generate_uuid() self.port.internal_info = {common.TENANT_VIF_KEY: vif_id} self.port.save() + pg_vif_id = uuidutils.generate_uuid() + portgroup = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, + address='52:54:00:00:00:00', + internal_info={common.TENANT_VIF_KEY: pg_vif_id}) + obj_utils.create_test_port( + self.context, node_id=self.node.id, portgroup_id=portgroup.id, + address='52:54:00:cf:2d:01', uuid=uuidutils.generate_uuid()) with task_manager.acquire(self.context, self.node.id) as task: vifs = self.interface.vif_list(task) - self.assertEqual([{'id': vif_id}], vifs) + self.assertItemsEqual([{'id': pg_vif_id}, {'id': vif_id}], vifs) def test_vif_list_extra_and_internal_priority(self): vif_id = uuidutils.generate_uuid() @@ -69,19 +244,42 @@ class TestVifPortIDMixin(db_base.DbTestCase): vifs = self.interface.vif_list(task) self.assertEqual([{'id': vif_id}], vifs) - @mock.patch.object(neutron_common, 'get_client') - @mock.patch.object(neutron_common, 'update_port_address') - def test_vif_attach(self, mock_upa, mock_client): + @mock.patch.object(common, 'get_free_port_like_object', autospec=True) + @mock.patch.object(neutron_common, 'get_client', autospec=True) + @mock.patch.object(neutron_common, 'update_port_address', autospec=True) + def test_vif_attach(self, mock_upa, mock_client, moc_gfp): self.port.extra = {} self.port.save() vif = {'id': "fake_vif_id"} + moc_gfp.return_value = self.port with task_manager.acquire(self.context, self.node.id) as task: self.interface.vif_attach(task, vif) self.port.refresh() self.assertEqual("fake_vif_id", self.port.internal_info.get( common.TENANT_VIF_KEY)) + mock_client.assert_called_once_with(None) mock_upa.assert_called_once_with("fake_vif_id", self.port.address) + @mock.patch.object(common, 'get_free_port_like_object', autospec=True) + @mock.patch.object(neutron_common, 'get_client') + @mock.patch.object(neutron_common, 'update_port_address') + def test_vif_attach_portgroup_no_address(self, mock_upa, mock_client, + mock_gfp): + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, address=None) + mock_gfp.return_value = pg + obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:01', + uuid=uuidutils.generate_uuid(), portgroup_id=pg.id) + vif = {'id': "fake_vif_id"} + with task_manager.acquire(self.context, self.node.id) as task: + self.interface.vif_attach(task, vif) + pg.refresh() + self.assertEqual(vif['id'], + pg.internal_info[common.TENANT_VIF_KEY]) + self.assertFalse(mock_upa.called) + self.assertFalse(mock_client.called) + @mock.patch.object(neutron_common, 'get_client') @mock.patch.object(neutron_common, 'update_port_address') def test_vif_attach_update_port_exception(self, mock_upa, mock_client): @@ -94,42 +292,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.assertRaisesRegexp( exception.NetworkError, "can not update Neutron port", self.interface.vif_attach, task, vif) - - def test_attach_port_no_ports_left_extra(self): - vif = {'id': "fake_vif_id"} - with task_manager.acquire(self.context, self.node.id) as task: - self.assertRaisesRegexp( - exception.NoFreePhysicalPorts, "not enough free physical", - self.interface.vif_attach, task, vif) - - def test_attach_port_no_ports_left_internal_info(self): - self.port.internal_info = { - common.TENANT_VIF_KEY: self.port.extra['vif_port_id']} - self.port.extra = {} - self.port.save() - vif = {'id': "fake_vif_id"} - with task_manager.acquire(self.context, self.node.id) as task: - self.assertRaisesRegexp( - exception.NoFreePhysicalPorts, "not enough free physical", - self.interface.vif_attach, task, vif) - - def test_attach_port_vif_already_attached_extra(self): - vif = {'id': self.port.extra['vif_port_id']} - with task_manager.acquire(self.context, self.node.id) as task: - self.assertRaisesRegexp( - exception.VifAlreadyAttached, "already attached", - self.interface.vif_attach, task, vif) - - def test_attach_port_vif_already_attach_internal_info(self): - vif = {'id': self.port.extra['vif_port_id']} - self.port.internal_info = { - common.TENANT_VIF_KEY: self.port.extra['vif_port_id']} - self.port.extra = {} - self.port.save() - with task_manager.acquire(self.context, self.node.id) as task: - self.assertRaisesRegexp( - exception.VifAlreadyAttached, "already attached", - self.interface.vif_attach, task, vif) + mock_client.assert_called_once_with(None) def test_vif_detach_in_extra(self): with task_manager.acquire(self.context, self.node.id) as task: @@ -151,6 +314,36 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.assertFalse('vif_port_id' in self.port.extra) self.assertFalse(common.TENANT_VIF_KEY in self.port.internal_info) + def test_vif_detach_in_extra_portgroup(self): + vif_id = uuidutils.generate_uuid() + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, + extra={'vif_port_id': vif_id}) + obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:01', + portgroup_id=pg.id, uuid=uuidutils.generate_uuid() + ) + with task_manager.acquire(self.context, self.node.id) as task: + self.interface.vif_detach(task, vif_id) + pg.refresh() + self.assertFalse('vif_port_id' in pg.extra) + self.assertFalse(common.TENANT_VIF_KEY in pg.internal_info) + + def test_vif_detach_in_internal_info_portgroup(self): + vif_id = uuidutils.generate_uuid() + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, + internal_info={common.TENANT_VIF_KEY: vif_id}) + obj_utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:01', + portgroup_id=pg.id, uuid=uuidutils.generate_uuid() + ) + with task_manager.acquire(self.context, self.node.id) as task: + self.interface.vif_detach(task, vif_id) + pg.refresh() + self.assertFalse('vif_port_id' in pg.extra) + self.assertFalse(common.TENANT_VIF_KEY in pg.internal_info) + def test_vif_detach_not_attached(self): with task_manager.acquire(self.context, self.node.id) as task: self.assertRaisesRegexp( @@ -466,6 +659,58 @@ class TestVifPortIDMixin(db_base.DbTestCase): mac_update_mock.assert_called_once_with('fake-id', new_address, token=self.context.auth_token) + @mock.patch.object(neutron_common, 'update_port_address', autospec=True) + def test_update_portgroup_remove_address(self, mac_update_mock): + pg = obj_utils.create_test_portgroup( + self.context, node_id=self.node.id, + extra={'vif_port_id': 'fake-id'}) + pg.address = None + with task_manager.acquire(self.context, self.node.id) as task: + 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( @@ -482,8 +727,10 @@ class TestVifPortIDMixin(db_base.DbTestCase): mac_update_mock.assert_called_once_with('fake-id', new_address, token=self.context.auth_token) + @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): + def test_update_portgroup_address_no_vif(self, mac_update_mock, mock_warn): pg = obj_utils.create_test_portgroup( self.context, node_id=self.node.id) new_address = '11:22:33:44:55:bb' @@ -492,6 +739,7 @@ class TestVifPortIDMixin(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/drivers/modules/network/test_flat.py b/ironic/tests/unit/drivers/modules/network/test_flat.py index a70a4dbf11..0ea06694bf 100644 --- a/ironic/tests/unit/drivers/modules/network/test_flat.py +++ b/ironic/tests/unit/drivers/modules/network/test_flat.py @@ -132,6 +132,29 @@ class TestFlatInterface(db_base.DbTestCase): self.interface.add_provisioning_network(task) upd_mock.assert_called_once_with('foo', exp_body) + @mock.patch.object(neutron, 'get_client') + def test_add_provisioning_network_set_binding_host_id_portgroup( + self, client_mock): + upd_mock = mock.Mock() + client_mock.return_value.update_port = upd_mock + instance_info = self.node.instance_info + instance_info['nova_host_id'] = 'nova_host_id' + self.node.instance_info = instance_info + self.node.save() + internal_info = {'tenant_vif_port_id': 'foo'} + utils.create_test_portgroup( + self.context, node_id=self.node.id, internal_info=internal_info, + uuid=uuidutils.generate_uuid()) + utils.create_test_port( + self.context, node_id=self.node.id, address='52:54:00:cf:2d:33', + extra={'vif_port_id': 'bar'}, uuid=uuidutils.generate_uuid()) + exp_body = {'port': {'binding:host_id': 'nova_host_id'}} + with task_manager.acquire(self.context, self.node.id) as task: + self.interface.add_provisioning_network(task) + upd_mock.assert_has_calls([ + mock.call('bar', exp_body), mock.call('foo', exp_body) + ]) + @mock.patch.object(neutron, 'get_client') def test_add_provisioning_network_no_binding_host_id( self, client_mock): diff --git a/releasenotes/notes/allow-attach-vif-to-portgroup-ce24b72632ec5772.yaml b/releasenotes/notes/allow-attach-vif-to-portgroup-ce24b72632ec5772.yaml new file mode 100644 index 0000000000..6a1d194570 --- /dev/null +++ b/releasenotes/notes/allow-attach-vif-to-portgroup-ce24b72632ec5772.yaml @@ -0,0 +1,11 @@ +--- +features: + - Enables port group usage when attaching/detaching VIFs. + When attaching a VIF to a node, it is attached to the first free port + group. Port group is considered free if it has no VIFs attached to any of + its ports. Otherwise, only the unattached ports of this portgroup are + available for attachment. If there are no free port groups, the first + available port (pxe_enabled has higher priority) is used instead. +deprecations: + - Using portgroup.extra['vif_port_id'] for attaching/detaching + VIFs to port groups is deprecated and will be removed in Pike release.