From b9ad3de19f648c159c371f78b1c7e068fe032a24 Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Tue, 24 Jan 2017 18:12:02 +0200 Subject: [PATCH] Allow to attach/detach VIFs to active ironic nodes This patch enhances attach/detach logic and allows to work with active ironic nodes. Starting with this change when VIF is attach/detach neutron port is updated appropriately to reflect changes on hardware (ie: plug port to specific network) Change-Id: Idc9dc37b11950e3aa2864a0a90d6ec2e307926a3 Related-Bug: #1659282 --- ironic/drivers/modules/network/common.py | 89 ++++++++++++++++++- ironic/drivers/modules/network/neutron.py | 59 ++---------- .../drivers/modules/network/test_common.py | 52 ++++++++++- ...h-vif-to-active-node-55963be2ec269043.yaml | 3 + 4 files changed, 143 insertions(+), 60 deletions(-) create mode 100644 releasenotes/notes/allow-to-attach-vif-to-active-node-55963be2ec269043.yaml diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index 3cf32ba37c..fd7213905e 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -23,6 +23,7 @@ from ironic.common import dhcp_factory from ironic.common import exception from ironic.common.i18n import _, _LW from ironic.common import neutron +from ironic.common import states from ironic.common import utils from ironic import objects @@ -137,6 +138,78 @@ def get_free_port_like_object(task, vif_id): return sorted_free_ports[0] +def plug_port_to_tenant_network(task, port_like_obj, client=None): + """Plug port like object to tenant network. + + :param task: A TaskManager instance. + :param port_like_obj: port-like object to plug. + :param client: Neutron client instance. + :raises NetworkError: if failed to update Neutron port. + :raises VifNotAttached if tenant VIF is not associated with port_like_obj. + """ + + node = task.node + local_link_info = [] + client_id_opt = None + + vif_id = ( + port_like_obj.internal_info.get(TENANT_VIF_KEY) or + port_like_obj.extra.get('vif_port_id')) + + if not vif_id: + obj_name = port_like_obj.__class__.__name__.lower() + raise exception.VifNotAttached( + _("Tenant VIF is not associated with %(obj_name)s " + "%(obj_id)s") % {'obj_name': obj_name, + 'obj_id': port_like_obj.uuid}) + + LOG.debug('Mapping tenant port %(vif_id)s to node ' + '%(node_id)s', + {'vif_id': vif_id, 'node_id': node.uuid}) + + if isinstance(port_like_obj, objects.Portgroup): + pg_ports = [p for p in task.ports + if p.portgroup_id == port_like_obj.id] + for port in pg_ports: + local_link_info.append(port.local_link_connection) + else: + # We iterate only on ports or portgroups, no need to check + # that it is a port + local_link_info.append(port_like_obj.local_link_connection) + client_id = port_like_obj.extra.get('client-id') + if client_id: + client_id_opt = ({'opt_name': 'client-id', 'opt_value': client_id}) + + # NOTE(sambetts) Only update required binding: attributes, + # because other port attributes may have been set by the user or + # nova. + body = { + 'port': { + 'binding:vnic_type': 'baremetal', + 'binding:host_id': node.uuid, + 'binding:profile': { + 'local_link_information': local_link_info, + }, + } + } + if client_id_opt: + body['port']['extra_dhcp_opts'] = [client_id_opt] + + if not client: + client = neutron.get_client() + + try: + client.update_port(vif_id, body) + except neutron_exceptions.ConnectionFailed as e: + msg = (_('Could not add public network VIF %(vif)s ' + 'to node %(node)s, possible network issue. %(exc)s') % + {'vif': vif_id, + 'node': node.uuid, + 'exc': e}) + LOG.error(msg) + raise exception.NetworkError(msg) + + class VIFPortIDMixin(object): def port_changed(self, task, port_obj): @@ -288,12 +361,12 @@ class VIFPortIDMixin(object): port_like_obj = get_free_port_like_object(task, vif_id) + client = neutron.get_client() # 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: - client = neutron.get_client() client.show_port(vif_id) except neutron_exceptions.NeutronClientException: # NOTE(sambetts): If a client error occurs this is because @@ -318,13 +391,17 @@ class VIFPortIDMixin(object): int_info[TENANT_VIF_KEY] = vif_id port_like_obj.internal_info = int_info port_like_obj.save() + # NOTE(vsaienko) allow to attach VIF to active instance. + if task.node.provision_state == states.ACTIVE: + plug_port_to_tenant_network(task, port_like_obj, client=client) def vif_detach(self, task, vif_id): """Detach a virtual network interface from a node :param task: A TaskManager instance. :param vif_id: A VIF ID to detach - :raises: VifNotAttached + :raises: VifNotAttached if VIF not attached. + :raises: NetworkError: if unbind Neutron port failed. """ ports = [p for p in task.ports if p.portgroup_id is None] @@ -332,8 +409,9 @@ class VIFPortIDMixin(object): 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_like_obj.extra.get("vif_port_id") == vif_id or - port_like_obj.internal_info.get(TENANT_VIF_KEY) == vif_id): + vif_port_id = port_like_obj.internal_info.get( + TENANT_VIF_KEY, port_like_obj.extra.get("vif_port_id")) + if vif_port_id == vif_id: int_info = port_like_obj.internal_info extra = port_like_obj.extra int_info.pop(TENANT_VIF_KEY, None) @@ -341,6 +419,9 @@ class VIFPortIDMixin(object): port_like_obj.extra = extra port_like_obj.internal_info = int_info port_like_obj.save() + # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance. + if task.node.provision_state == states.ACTIVE: + neutron.unbind_neutron_port(vif_port_id) break else: raise exception.VifNotAttached(vif=vif_id, node=task.node.uuid) diff --git a/ironic/drivers/modules/network/neutron.py b/ironic/drivers/modules/network/neutron.py index 9b70997b8e..589f4b859d 100644 --- a/ironic/drivers/modules/network/neutron.py +++ b/ironic/drivers/modules/network/neutron.py @@ -14,7 +14,6 @@ # under the License. -from neutronclient.common import exceptions as neutron_exceptions from oslo_config import cfg from oslo_log import log @@ -23,7 +22,6 @@ from ironic.common.i18n import _, _LI from ironic.common import neutron from ironic.drivers import base from ironic.drivers.modules.network import common -from ironic import objects LOG = log.getLogger(__name__) @@ -160,63 +158,16 @@ class NeutronNetwork(common.VIFPortIDMixin, ports = [p for p in ports if not p.portgroup_id] portgroups = task.portgroups - portmap = neutron.get_node_portmap(task) - client = neutron.get_client() pobj_without_vif = 0 for port_like_obj in ports + portgroups: - vif_port_id = ( - port_like_obj.internal_info.get(common.TENANT_VIF_KEY) or - port_like_obj.extra.get('vif_port_id')) - - if not vif_port_id: - pobj_without_vif += 1 - continue - - LOG.debug('Mapping tenant port %(vif_port_id)s to node ' - '%(node_id)s', - {'vif_port_id': vif_port_id, 'node_id': node.uuid}) - local_link_info = [] - client_id_opt = None - if isinstance(port_like_obj, objects.Portgroup): - pg_ports = [p for p in task.ports - if p.portgroup_id == port_like_obj.id] - for port in pg_ports: - local_link_info.append(portmap[port.uuid]) - else: - # We iterate only on ports or portgroups, no need to check - # that it is a port - local_link_info.append(portmap[port_like_obj.uuid]) - client_id = port_like_obj.extra.get('client-id') - if client_id: - client_id_opt = ( - {'opt_name': 'client-id', 'opt_value': client_id}) - - # NOTE(sambetts) Only update required binding: attributes, - # because other port attributes may have been set by the user or - # nova. - body = { - 'port': { - 'binding:vnic_type': 'baremetal', - 'binding:host_id': node.uuid, - 'binding:profile': { - 'local_link_information': local_link_info, - }, - } - } - if client_id_opt: - body['port']['extra_dhcp_opts'] = [client_id_opt] try: - client.update_port(vif_port_id, body) - except neutron_exceptions.ConnectionFailed as e: - msg = (_('Could not add public network VIF %(vif)s ' - 'to node %(node)s, possible network issue. %(exc)s') % - {'vif': vif_port_id, - 'node': node.uuid, - 'exc': e}) - LOG.error(msg) - raise exception.NetworkError(msg) + common.plug_port_to_tenant_network(task, port_like_obj, + client=client) + except exception.VifNotAttached: + pobj_without_vif += 1 + continue if pobj_without_vif == len(ports + portgroups): msg = _("No neutron ports or portgroups are associated with " diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index 67d17f528f..5e77fdb699 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -16,6 +16,7 @@ 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 @@ -236,6 +237,36 @@ class TestCommonFunctions(db_base.DbTestCase): common.get_free_port_like_object, task, self.vif_id) + @mock.patch.object(neutron_common, 'get_client', autospec=True) + def test_plug_port_to_tenant_network_client(self, mock_gc): + 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: + common.plug_port_to_tenant_network(task, self.port, + client=mock.MagicMock()) + self.assertFalse(mock_gc.called) + + @mock.patch.object(neutron_common, 'get_client', autospec=True) + def test_plug_port_to_tenant_network_no_client(self, mock_gc): + 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: + common.plug_port_to_tenant_network(task, self.port) + self.assertTrue(mock_gc.called) + + @mock.patch.object(neutron_common, 'get_client', autospec=True) + def test_plug_port_to_tenant_network_no_tenant_vif(self, mock_gc): + nclient = mock.MagicMock() + mock_gc.return_value = nclient + self.port.extra = {} + self.port.save() + with task_manager.acquire(self.context, self.node.id) as task: + self.assertRaisesRegex( + exception.VifNotAttached, + "not associated with port %s" % self.port.uuid, + common.plug_port_to_tenant_network, + task, self.port) + class TestVifPortIDMixin(db_base.DbTestCase): @@ -330,7 +361,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.assertEqual(vif['id'], pg.internal_info[common.TENANT_VIF_KEY]) self.assertFalse(mock_upa.called) - self.assertFalse(mock_client.called) + self.assertTrue(mock_client.called) @mock.patch.object(neutron_common, 'get_client') @mock.patch.object(neutron_common, 'update_port_address') @@ -354,7 +385,8 @@ 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_internal_info(self): + @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True) + def test_vif_detach_in_internal_info(self, mock_unp): self.port.internal_info = { common.TENANT_VIF_KEY: self.port.extra['vif_port_id']} self.port.extra = {} @@ -365,6 +397,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.port.refresh() self.assertFalse('vif_port_id' in self.port.extra) self.assertFalse(common.TENANT_VIF_KEY in self.port.internal_info) + self.assertFalse(mock_unp.called) def test_vif_detach_in_extra_portgroup(self): vif_id = uuidutils.generate_uuid() @@ -402,6 +435,21 @@ class TestVifPortIDMixin(db_base.DbTestCase): exception.VifNotAttached, "it is not attached to it.", self.interface.vif_detach, task, 'aaa') + @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True) + def test_vif_detach_active_node(self, mock_unp): + vif_id = self.port.extra['vif_port_id'] + self.port.internal_info = { + common.TENANT_VIF_KEY: vif_id} + self.port.extra = {} + self.port.save() + self.node.provision_state = states.ACTIVE + self.node.save() + with task_manager.acquire(self.context, self.node.id) as task: + self.interface.vif_detach( + task, self.port.internal_info[common.TENANT_VIF_KEY]) + self.port.refresh() + mock_unp.assert_called_once_with(vif_id) + def test_get_current_vif_extra_vif_port_id(self): extra = {'vif_port_id': 'foo'} self.port.extra = extra diff --git a/releasenotes/notes/allow-to-attach-vif-to-active-node-55963be2ec269043.yaml b/releasenotes/notes/allow-to-attach-vif-to-active-node-55963be2ec269043.yaml new file mode 100644 index 0000000000..00a0745649 --- /dev/null +++ b/releasenotes/notes/allow-to-attach-vif-to-active-node-55963be2ec269043.yaml @@ -0,0 +1,3 @@ +--- +features: + - Adds possibility to attach/detach VIFs to/from active nodes.