From 8a4c8da4bb6b5d71dd033abca5f6e88c621475a5 Mon Sep 17 00:00:00 2001 From: Ryota MIBU Date: Mon, 12 Aug 2013 14:10:40 +0900 Subject: [PATCH] Fix resource status in NEC Plugin This commit makes sure that the plugin exposes right status in a response body, and does not overwrite ERROR status until another operation to the backend has succeeded. This commit also changes NEC Plguin to use neutron constants instead of OperationalStatus defined in this plugin. Fixes: bug #1211319 Change-Id: Ic61b8e1b9d3f6c2be9567dd5a4606aa6d439c564 --- neutron/plugins/nec/nec_plugin.py | 167 ++++++++++------------ neutron/tests/unit/nec/test_nec_plugin.py | 128 ++++++++++++++++- 2 files changed, 200 insertions(+), 95 deletions(-) diff --git a/neutron/plugins/nec/nec_plugin.py b/neutron/plugins/nec/nec_plugin.py index 45ea05366f..acb384af3c 100644 --- a/neutron/plugins/nec/nec_plugin.py +++ b/neutron/plugins/nec/nec_plugin.py @@ -19,7 +19,7 @@ from neutron.agent import securitygroups_rpc as sg_rpc from neutron.api.rpc.agentnotifiers import dhcp_rpc_agent_api from neutron.api.rpc.agentnotifiers import l3_rpc_agent_api -from neutron.common import constants as q_const +from neutron.common import constants as const from neutron.common import exceptions as q_exc from neutron.common import rpc as q_rpc from neutron.common import topics @@ -38,6 +38,7 @@ from neutron.openstack.common import importutils from neutron.openstack.common import log as logging from neutron.openstack.common import rpc from neutron.openstack.common.rpc import proxy +from neutron.openstack.common import uuidutils from neutron.plugins.nec.common import config from neutron.plugins.nec.common import exceptions as nexc from neutron.plugins.nec.db import api as ndb @@ -47,21 +48,6 @@ from neutron.plugins.nec import packet_filter LOG = logging.getLogger(__name__) -class OperationalStatus: - """Enumeration for operational status. - - ACTIVE: The resource is available. - DOWN: The resource is not operational. This might indicate - admin_state_up=False, or lack of OpenFlow info for the port. - BUILD: The plugin is creating the resource. - ERROR: Some error occured. - """ - ACTIVE = "ACTIVE" - DOWN = "DOWN" - BUILD = "BUILD" - ERROR = "ERROR" - - class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, extraroute_db.ExtraRoute_db_mixin, l3_gwmode_db.L3_NAT_db_mixin, @@ -123,10 +109,10 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, self.topic = topics.PLUGIN self.conn = rpc.create_connection(new=True) self.notifier = NECPluginV2AgentNotifierApi(topics.AGENT) - self.agent_notifiers[q_const.AGENT_TYPE_DHCP] = ( + self.agent_notifiers[const.AGENT_TYPE_DHCP] = ( dhcp_rpc_agent_api.DhcpAgentNotifyAPI() ) - self.agent_notifiers[q_const.AGENT_TYPE_L3] = ( + self.agent_notifiers[const.AGENT_TYPE_L3] = ( l3_rpc_agent_api.L3AgentNotify ) @@ -151,7 +137,6 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, def activate_port_if_ready(self, context, port, network=None): """Activate port by creating port on OFC if ready. - Activate port and packet_filters associated with the port. Conditions to activate port on OFC are: * port admin_state is UP * network admin_state is UP @@ -161,52 +146,29 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, network = super(NECPluginV2, self).get_network(context, port['network_id']) - port_status = OperationalStatus.ACTIVE if not port['admin_state_up']: LOG.debug(_("activate_port_if_ready(): skip, " "port.admin_state_up is False.")) - port_status = OperationalStatus.DOWN + return port elif not network['admin_state_up']: LOG.debug(_("activate_port_if_ready(): skip, " "network.admin_state_up is False.")) - port_status = OperationalStatus.DOWN + return port elif not ndb.get_portinfo(context.session, port['id']): LOG.debug(_("activate_port_if_ready(): skip, " "no portinfo for this port.")) - port_status = OperationalStatus.DOWN + return port + elif self.ofc.exists_ofc_port(context, port['id']): + LOG.debug(_("activate_port_if_ready(): skip, " + "ofc_port already exists.")) + return port - if port_status in [OperationalStatus.ACTIVE]: - if self.ofc.exists_ofc_port(context, port['id']): - LOG.debug(_("activate_port_if_ready(): skip, " - "ofc_port already exists.")) - else: - try: - self.ofc.create_ofc_port(context, port['id'], port) - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: - reason = _("create_ofc_port() failed due to %s") % exc - LOG.error(reason) - port_status = OperationalStatus.ERROR - - if port_status is not port['status']: - self._update_resource_status(context, "port", port['id'], - port_status) - - def deactivate_port(self, context, port): - """Deactivate port by deleting port from OFC if exists. - - Deactivate port and packet_filters associated with the port. - """ - port_status = OperationalStatus.DOWN - if self.ofc.exists_ofc_port(context, port['id']): - try: - self.ofc.delete_ofc_port(context, port['id'], port) - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: - reason = _("delete_ofc_port() failed due to %s") % exc - LOG.error(reason) - port_status = OperationalStatus.ERROR - else: - LOG.debug(_("deactivate_port(): skip, ofc_port does not " - "exist.")) + try: + self.ofc.create_ofc_port(context, port['id'], port) + port_status = const.PORT_STATUS_ACTIVE + except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + LOG.error(_("create_ofc_port() failed due to %s"), exc) + port_status = const.PORT_STATUS_ERROR if port_status is not port['status']: self._update_resource_status(context, "port", port['id'], @@ -215,34 +177,62 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, return port + def deactivate_port(self, context, port): + """Deactivate port by deleting port from OFC if exists.""" + if not self.ofc.exists_ofc_port(context, port['id']): + LOG.debug(_("deactivate_port(): skip, ofc_port does not " + "exist.")) + return port + + try: + self.ofc.delete_ofc_port(context, port['id'], port) + port_status = const.PORT_STATUS_DOWN + except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + LOG.error(_("delete_ofc_port() failed due to %s"), exc) + port_status = const.PORT_STATUS_ERROR + + if port_status is not port['status']: + self._update_resource_status(context, "port", port['id'], + port_status) + port['status'] = port_status + + return port + + def _net_status(self, network): + # NOTE: NEC Plugin accept admin_state_up. When it's False, this plugin + # deactivate all ports on the network to drop all packet and show + # status='DOWN' to users. But the network is kept defined on OFC. + if network['network']['admin_state_up']: + return const.NET_STATUS_ACTIVE + else: + return const.NET_STATUS_DOWN + def create_network(self, context, network): """Create a new network entry on DB, and create it on OFC.""" LOG.debug(_("NECPluginV2.create_network() called, " "network=%s ."), network) + tenant_id = self._get_tenant_id_for_create(context, network['network']) + net_name = network['network']['name'] + net_id = uuidutils.generate_uuid() + #set up default security groups - tenant_id = self._get_tenant_id_for_create( - context, network['network']) self._ensure_default_security_group(context, tenant_id) + network['network']['id'] = net_id + network['network']['status'] = self._net_status(network) + + try: + if not self.ofc.exists_ofc_tenant(context, tenant_id): + self.ofc.create_ofc_tenant(context, tenant_id) + self.ofc.create_ofc_network(context, tenant_id, net_id, net_name) + except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + LOG.error(_("failed to create network id=%(id)s on " + "OFC: %(exc)s"), {'id': net_id, 'exc': exc}) + network['network']['status'] = const.NET_STATUS_ERROR + with context.session.begin(subtransactions=True): new_net = super(NECPluginV2, self).create_network(context, network) self._process_l3_create(context, new_net, network['network']) - self._update_resource_status(context, "network", new_net['id'], - OperationalStatus.BUILD) - - try: - if not self.ofc.exists_ofc_tenant(context, new_net['tenant_id']): - self.ofc.create_ofc_tenant(context, new_net['tenant_id']) - self.ofc.create_ofc_network(context, new_net['tenant_id'], - new_net['id'], new_net['name']) - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: - reason = _("create_network() failed due to %s") % exc - LOG.error(reason) - self._update_resource_status(context, "network", new_net['id'], - OperationalStatus.ERROR) - else: - self._update_resource_status(context, "network", new_net['id'], - OperationalStatus.ACTIVE) return new_net @@ -255,6 +245,10 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, LOG.debug(_("NECPluginV2.update_network() called, " "id=%(id)s network=%(network)s ."), {'id': id, 'network': network}) + + if 'admin_state_up' in network['network']: + network['network']['status'] = self._net_status(network) + session = context.session with session.begin(subtransactions=True): old_net = super(NECPluginV2, self).get_network(context, id) @@ -264,19 +258,15 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, changed = (old_net['admin_state_up'] is not new_net['admin_state_up']) if changed and not new_net['admin_state_up']: - self._update_resource_status(context, "network", id, - OperationalStatus.DOWN) # disable all active ports of the network - filters = dict(network_id=[id], status=[OperationalStatus.ACTIVE]) + filters = dict(network_id=[id], status=[const.PORT_STATUS_ACTIVE]) ports = super(NECPluginV2, self).get_ports(context, filters=filters) for port in ports: self.deactivate_port(context, port) elif changed and new_net['admin_state_up']: - self._update_resource_status(context, "network", id, - OperationalStatus.ACTIVE) # enable ports of the network - filters = dict(network_id=[id], status=[OperationalStatus.DOWN], + filters = dict(network_id=[id], status=[const.PORT_STATUS_DOWN], admin_state_up=[True]) ports = super(NECPluginV2, self).get_ports(context, filters=filters) @@ -308,7 +298,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, _error_ports = [] for port in ports: port = self.deactivate_port(context, port) - if port['status'] == OperationalStatus.ERROR: + if port['status'] == const.PORT_STATUS_ERROR: _error_ports.append(port['id']) if _error_ports: reason = (_("Failed to delete port(s)=%s from OFC.") % @@ -329,7 +319,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, reason = _("delete_network() failed due to %s") % exc LOG.error(reason) self._update_resource_status(context, "network", net['id'], - OperationalStatus.ERROR) + const.NET_STATUS_ERROR) raise super(NECPluginV2, self).delete_network(context, id) @@ -355,6 +345,9 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, def create_port(self, context, port): """Create a new port entry on DB, then try to activate it.""" LOG.debug(_("NECPluginV2.create_port() called, port=%s ."), port) + + port['port']['status'] = const.PORT_STATUS_DOWN + port_data = port['port'] with context.session.begin(subtransactions=True): self._ensure_default_security_group_on_port(context, port) @@ -366,10 +359,8 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, self._process_port_create_security_group( context, port, sgids) self.notify_security_groups_member_updated(context, port) - self._update_resource_status(context, "port", port['id'], - OperationalStatus.BUILD) - self.activate_port_if_ready(context, port) - return port + + return self.activate_port_if_ready(context, port) def update_port(self, context, id, port): """Update port, and handle packetfilters associated with the port. @@ -398,9 +389,9 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, changed = (old_port['admin_state_up'] != new_port['admin_state_up']) if changed: if new_port['admin_state_up']: - self.activate_port_if_ready(context, new_port) + new_port = self.activate_port_if_ready(context, new_port) else: - self.deactivate_port(context, old_port) + new_port = self.deactivate_port(context, new_port) return new_port @@ -413,7 +404,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, port = self.get_port(context, id) port = self.deactivate_port(context, port) - if port['status'] == OperationalStatus.ERROR: + if port['status'] == const.PORT_STATUS_ERROR: reason = _("Failed to delete port=%s from OFC.") % id raise nexc.OFCException(reason=reason) diff --git a/neutron/tests/unit/nec/test_nec_plugin.py b/neutron/tests/unit/nec/test_nec_plugin.py index 9a754792d9..c597692230 100644 --- a/neutron/tests/unit/nec/test_nec_plugin.py +++ b/neutron/tests/unit/nec/test_nec_plugin.py @@ -19,6 +19,7 @@ import fixtures import mock import webob.exc +from neutron.common import constants from neutron.common.test_lib import test_config from neutron.common import topics from neutron import context @@ -86,6 +87,8 @@ class NecPluginV2TestCase(test_plugin.NeutronDbPluginV2TestCase): self.ofc = self.plugin.ofc self.callback_nec = nec_plugin.NECPluginV2RPCCallbacks(self.plugin) self.context = context.get_admin_context() + self.net_create_status = 'ACTIVE' + self.port_create_status = 'DOWN' class TestNecBasicGet(test_plugin.TestBasicGet, NecPluginV2TestCase): @@ -312,7 +315,7 @@ class TestNecPluginDbTest(NecPluginV2TestCase): for status in ["DOWN", "BUILD", "ERROR", "ACTIVE"]: self.plugin._update_resource_status( self.context, 'network', net_id, - getattr(nec_plugin.OperationalStatus, status)) + getattr(constants, 'NET_STATUS_%s' % status)) n = self.plugin._get_network(self.context, net_id) self.assertEqual(status, n.status) @@ -376,6 +379,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): ctx = mock.ANY with self.network(admin_state_up=False) as network: net = network['network'] + self.assertEqual(network['network']['status'], 'DOWN') expected = [ mock.call.exists_ofc_tenant(ctx, self._tenant_id), @@ -423,6 +427,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): # network from OFC. Deletion of network is not the scope of this test. with self.network(do_delete=False) as network: net = network['network'] + self.assertEqual(net['status'], 'ERROR') net_ref = self._show('networks', net['id']) self.assertEqual(net_ref['network']['status'], 'ERROR') @@ -441,15 +446,26 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): net = network['network'] self.assertEqual(network['network']['status'], 'ACTIVE') + net_ref = self._show('networks', net['id']) + self.assertEqual(net_ref['network']['status'], 'ACTIVE') + # Set admin_state_up to False res = self._update_resource('network', net['id'], {'admin_state_up': False}) self.assertFalse(res['admin_state_up']) + self.assertEqual(res['status'], 'DOWN') + + net_ref = self._show('networks', net['id']) + self.assertEqual(net_ref['network']['status'], 'DOWN') # Set admin_state_up to True res = self._update_resource('network', net['id'], {'admin_state_up': True}) self.assertTrue(res['admin_state_up']) + self.assertEqual(res['status'], 'ACTIVE') + + net_ref = self._show('networks', net['id']) + self.assertEqual(net_ref['network']['status'], 'ACTIVE') expected = [ mock.call.exists_ofc_tenant(ctx, self._tenant_id), @@ -471,7 +487,10 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): net_id = port['port']['network_id'] net = self._show_resource('network', net_id) self.assertEqual(net['status'], 'ACTIVE') - self.assertEqual(p1['status'], 'ACTIVE') + self.assertEqual(p1['status'], 'DOWN') + + p1_ref = self._show('ports', p1['id']) + self.assertEqual(p1_ref['port']['status'], 'DOWN') expected = [ mock.call.exists_ofc_tenant(ctx, self._tenant_id), @@ -495,7 +514,10 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): net_id = port['port']['network_id'] net = self._show_resource('network', net_id) self.assertEqual(net['status'], 'ACTIVE') - self.assertEqual(p1['status'], 'ACTIVE') + self.assertEqual(p1['status'], 'DOWN') + + p1_ref = self._show('ports', p1['id']) + self.assertEqual(p1_ref['port']['status'], 'DOWN') # Check the port is not created on OFC self.assertFalse(self.ofc.create_ofc_port.call_count) @@ -505,6 +527,9 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): self.rpcapi_update_ports(added=[portinfo]) self.assertEqual(self.ofc.create_ofc_port.call_count, 1) + p1_ref = self._show('ports', p1['id']) + self.assertEqual(p1_ref['port']['status'], 'ACTIVE') + expected = [ mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.create_ofc_tenant(ctx, self._tenant_id), @@ -646,6 +671,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): p1 = port['port'] net_id = port['port']['network_id'] res_id = net_id if resource == 'network' else p1['id'] + self.assertEqual(p1['status'], 'DOWN') net = self._show_resource('network', net_id) @@ -657,13 +683,15 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): self.rpcapi_update_ports(added=[portinfo]) self.assertFalse(self.ofc.create_ofc_port.call_count) - self._update_resource(resource, res_id, - {'admin_state_up': True}) + res = self._update_resource(resource, res_id, + {'admin_state_up': True}) + self.assertEqual(res['status'], 'ACTIVE') self.assertEqual(self.ofc.create_ofc_port.call_count, 1) self.assertFalse(self.ofc.delete_ofc_port.call_count) - self._update_resource(resource, res_id, - {'admin_state_up': False}) + res = self._update_resource(resource, res_id, + {'admin_state_up': False}) + self.assertEqual(res['status'], 'DOWN') self.assertEqual(self.ofc.delete_ofc_port.call_count, 1) expected = [ @@ -684,6 +712,92 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): ] self.ofc.assert_has_calls(expected) + def test_update_port_with_ofc_creation_failure(self): + with self.port(admin_state_up=False) as port: + port_id = port['port']['id'] + portinfo = {'id': port_id, 'port_no': 123} + self.rpcapi_update_ports(added=[portinfo]) + + self.ofc.set_raise_exc('create_ofc_port', + nexc.OFCException(reason='hoge')) + + body = {'port': {'admin_state_up': True}} + res = self._update('ports', port_id, body) + self.assertEqual(res['port']['status'], 'ERROR') + port_ref = self._show('ports', port_id) + self.assertEqual(port_ref['port']['status'], 'ERROR') + + body = {'port': {'admin_state_up': False}} + res = self._update('ports', port_id, body) + self.assertEqual(res['port']['status'], 'ERROR') + port_ref = self._show('ports', port_id) + self.assertEqual(port_ref['port']['status'], 'ERROR') + + self.ofc.set_raise_exc('create_ofc_port', None) + + body = {'port': {'admin_state_up': True}} + res = self._update('ports', port_id, body) + self.assertEqual(res['port']['status'], 'ACTIVE') + port_ref = self._show('ports', port_id) + self.assertEqual(port_ref['port']['status'], 'ACTIVE') + + ctx = mock.ANY + port = mock.ANY + expected = [ + mock.call.exists_ofc_port(ctx, port_id), + mock.call.create_ofc_port(ctx, port_id, port), + mock.call.exists_ofc_port(ctx, port_id), + mock.call.exists_ofc_port(ctx, port_id), + mock.call.create_ofc_port(ctx, port_id, port), + mock.call.exists_ofc_port(ctx, port_id), + mock.call.delete_ofc_port(ctx, port_id, port), + ] + self.ofc.assert_has_calls(expected) + self.assertEqual(self.ofc.create_ofc_port.call_count, 2) + + def test_update_port_with_ofc_deletion_failure(self): + with self.port() as port: + port_id = port['port']['id'] + portinfo = {'id': port_id, 'port_no': 123} + self.rpcapi_update_ports(added=[portinfo]) + + self.ofc.set_raise_exc('delete_ofc_port', + nexc.OFCException(reason='hoge')) + + body = {'port': {'admin_state_up': False}} + res = self._update('ports', port_id, body) + self.assertEqual(res['port']['status'], 'ERROR') + port_ref = self._show('ports', port_id) + self.assertEqual(port_ref['port']['status'], 'ERROR') + + body = {'port': {'admin_state_up': True}} + res = self._update('ports', port_id, body) + self.assertEqual(res['port']['status'], 'ERROR') + port_ref = self._show('ports', port_id) + self.assertEqual(port_ref['port']['status'], 'ERROR') + + self.ofc.set_raise_exc('delete_ofc_port', None) + + body = {'port': {'admin_state_up': False}} + res = self._update('ports', port_id, body) + self.assertEqual(res['port']['status'], 'DOWN') + port_ref = self._show('ports', port_id) + self.assertEqual(port_ref['port']['status'], 'DOWN') + + ctx = mock.ANY + port = mock.ANY + expected = [ + mock.call.exists_ofc_port(ctx, port_id), + mock.call.create_ofc_port(ctx, port_id, port), + mock.call.exists_ofc_port(ctx, port_id), + mock.call.delete_ofc_port(ctx, port_id, port), + mock.call.exists_ofc_port(ctx, port_id), + mock.call.exists_ofc_port(ctx, port_id), + mock.call.delete_ofc_port(ctx, port_id, port), + ] + self.ofc.assert_has_calls(expected) + self.assertEqual(self.ofc.delete_ofc_port.call_count, 2) + def test_delete_port_with_ofc_deletion_failure(self): self.ofc.set_raise_exc('delete_ofc_port', nexc.OFCException(reason='hoge'))