Cisco VPN device driver post-merge cleanup

During review for I-3 there were some minor comments regarding log
level on message and asserts for mock calls.

In addition, some debug logging enhancements were made between the
service and device driver to better indicate the source of the
shared "vpnservice_updated" RPC.

Lastly, unit tests were updated, based on a newer Cisco CSR image,
which had REST API fixes and behavior changes.

Change-Id: I22277462270df0b2cd642ec576e8652c9df146b5
Closes-bug: 1288387

diff --git a/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py b/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py
index 3cc3c435f2ec7fa15f4a3f8e998b108952210bd7..2e55992836d7da355580e3af9dce3c6cc53a9d8e 100644 (file)
--- a/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py
+++ b/neutron/services/vpn/device_drivers/cisco_csr_rest_client.py
@@ -102,10 +102,9 @@ class CsrRestClient(object):
                          if isinstance(te, r_exc.SSLError) else '',
                          'host': self.host})
             self.status = requests.codes.REQUEST_TIMEOUT
-        except r_exc.ConnectionError as ce:
-            LOG.error(_("%(method)s: Unable to connect to CSR(%(host)s): "
-                        "%(error)s"),
-                      {'method': method, 'host': self.host, 'error': ce})
+        except r_exc.ConnectionError:
+            LOG.exception(_("%(method)s: Unable to connect to CSR(%(host)s)"),
+                          {'method': method, 'host': self.host})
             self.status = requests.codes.NOT_FOUND
         except Exception as e:
             LOG.error(_("%(method)s: Unexpected error for CSR (%(host)s): "

diff --git a/neutron/services/vpn/device_drivers/cisco_ipsec.py b/neutron/services/vpn/device_drivers/cisco_ipsec.py
index c518491d3ef0732e3e6c3b35f97a0c7e69d6d777..5bbe5628b86448a57d00aa7ba98540d24a886d83 100644 (file)
--- a/neutron/services/vpn/device_drivers/cisco_ipsec.py
+++ b/neutron/services/vpn/device_drivers/cisco_ipsec.py
@@ -16,7 +16,7 @@
 
 import abc
 from collections import namedtuple
-import httplib
+import requests
 
 import netaddr
 from oslo.config import cfg
@@ -221,7 +221,8 @@ class CiscoCsrIPsecDriver(device_drivers.DeviceDriver):
 
     def vpnservice_updated(self, context, **kwargs):
         """Handle VPNaaS service driver change notifications."""
-        LOG.debug(_("Handling VPN service update notification"))
+        LOG.debug(_("Handling VPN service update notification '%s'"),
+                  kwargs.get('reason', ''))
         self.sync(context, [])
 
     def create_vpn_service(self, service_data):
@@ -675,7 +676,7 @@ class CiscoCsrIPSecConnection(object):
 
     def _check_create(self, resource, which):
         """Determine if REST create request was successful."""
-        if self.csr.status == httplib.CREATED:
+        if self.csr.status == requests.codes.CREATED:
             LOG.debug("%(resource)s %(which)s is configured",
                       {'resource': resource, 'which': which})
             return
@@ -701,7 +702,7 @@ class CiscoCsrIPSecConnection(object):
 
     def _verify_deleted(self, status, resource, which):
         """Determine if REST delete request was successful."""
-        if status in (httplib.NO_CONTENT, httplib.NOT_FOUND):
+        if status in (requests.codes.NO_CONTENT, requests.codes.NOT_FOUND):
             LOG.debug("%(resource)s configuration %(which)s was removed",
                       {'resource': resource, 'which': which})
         else:

diff --git a/neutron/services/vpn/service_drivers/__init__.py b/neutron/services/vpn/service_drivers/__init__.py
index c932de71632e84a6c88e0bdc73100271943d06b5..9e7484ed58e15161eca52df93cb75640184505e1 100644 (file)
--- a/neutron/services/vpn/service_drivers/__init__.py
+++ b/neutron/services/vpn/service_drivers/__init__.py
@@ -76,7 +76,7 @@ class BaseIPsecVpnAgentApi(proxy.RpcProxy):
             active=True)
         for l3_agent in l3_agents:
             LOG.debug(_('Notify agent at %(topic)s.%(host)s the message '
-                        '%(method)s'),
+                        '%(method)s %(args)s'),
                       {'topic': self.to_agent_topic,
                        'host': l3_agent.host,
                        'method': method,
@@ -86,6 +86,7 @@ class BaseIPsecVpnAgentApi(proxy.RpcProxy):
                 version=version,
                 topic='%s.%s' % (self.to_agent_topic, l3_agent.host))
 
-    def vpnservice_updated(self, context, router_id):
+    def vpnservice_updated(self, context, router_id, **kwargs):
         """Send update event of vpnservices."""
-        self._agent_notification(context, 'vpnservice_updated', router_id)
+        self._agent_notification(context, 'vpnservice_updated', router_id,
+                                 **kwargs)

diff --git a/neutron/services/vpn/service_drivers/cisco_ipsec.py b/neutron/services/vpn/service_drivers/cisco_ipsec.py
index 625fea93696fcce1a4f1f8009507ecde1b9e4ba5..4afd71b64ef3a46f9bbf9d10f5c8335c3f270852 100644 (file)
--- a/neutron/services/vpn/service_drivers/cisco_ipsec.py
+++ b/neutron/services/vpn/service_drivers/cisco_ipsec.py
@@ -179,7 +179,8 @@ class CiscoCsrIPsecVPNDriver(service_drivers.VpnDriver):
                 self.service_plugin.update_ipsec_site_conn_status(
                     context, ipsec_site_connection['id'], constants.ERROR)
         csr_id_map.create_tunnel_mapping(context, ipsec_site_connection)
-        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'])
+        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'],
+                                          reason='ipsec-conn-create')
 
     def update_ipsec_site_connection(
         self, context, old_ipsec_site_connection, ipsec_site_connection):
@@ -190,7 +191,8 @@ class CiscoCsrIPsecVPNDriver(service_drivers.VpnDriver):
     def delete_ipsec_site_connection(self, context, ipsec_site_connection):
         vpnservice = self.service_plugin._get_vpnservice(
             context, ipsec_site_connection['vpnservice_id'])
-        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'])
+        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'],
+                                          reason='ipsec-conn-delete')
 
     def create_ikepolicy(self, context, ikepolicy):
         pass
@@ -214,10 +216,12 @@ class CiscoCsrIPsecVPNDriver(service_drivers.VpnDriver):
         pass
 
     def update_vpnservice(self, context, old_vpnservice, vpnservice):
-        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'])
+        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'],
+                                          reason='vpn-service-update')
 
     def delete_vpnservice(self, context, vpnservice):
-        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'])
+        self.agent_rpc.vpnservice_updated(context, vpnservice['router_id'],
+                                          reason='vpn-service-delete')
 
     def get_cisco_connection_mappings(self, conn_id, context):
         """Obtain persisted mappings for IDs related to connection."""

diff --git a/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py b/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py
index 3830303030ad6d5d9b3b666b9dae20d4ccc6bd40..84f5d4ca5cbb1012b3149974909fa97a7a4fbbbc 100644 (file)
--- a/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py
+++ b/neutron/tests/unit/services/vpn/device_drivers/cisco_csr_mock.py
@@ -29,7 +29,6 @@ from neutron.openstack.common import log as logging
 from neutron.tests.unit.services.vpn.device_drivers import httmock
 
 # TODO(pcm) Remove, once verified these have been fixed
-FIXED_CSCum50512 = False
 FIXED_CSCum35484 = False
 FIXED_CSCul82396 = False
 FIXED_CSCum10324 = False
@@ -236,7 +235,7 @@ def normal_get(url, request):
                    u'outgoing-interface': u'GigabitEthernet1',
                    u'admin-distance': 1}
         return httmock.response(requests.codes.OK, content=content)
-    if 'vpn-svc/site-to-site/active/sessions':
+    if 'vpn-svc/site-to-site/active/sessions' in url.path:
         # Only including needed fields for mock
         content = {u'kind': u'collection#vpn-active-sessions',
                    u'items': [{u'status': u'DOWN-NEGOTIATING',
@@ -318,26 +317,23 @@ def get_defaults(url, request):
 def get_unnumbered(url, request):
     if not request.headers.get('X-auth-token', None):
         return {'status_code': requests.codes.UNAUTHORIZED}
-    if FIXED_CSCum50512:
-        tunnel = url.path.split('/')[-1]
-        ipsec_policy_id = tunnel[6:]
-        content = {u'kind': u'object#vpn-site-to-site',
-                   u'vpn-interface-name': u'%s' % tunnel,
-                   u'ip-version': u'ipv4',
-                   u'vpn-type': u'site-to-site',
-                   u'ipsec-policy-id': u'%s' % ipsec_policy_id,
-                   u'ike-profile-id': None,
-                   u'mtu': 1500,
-                   u'local-device': {
-                       u'ip-address': u'unnumbered GigabitEthernet3',
-                       u'tunnel-ip-address': u'10.10.10.10'
-                   },
-                   u'remote-device': {
-                       u'tunnel-ip-address': u'10.10.10.20'
-                   }}
-        return httmock.response(requests.codes.OK, content=content)
-    else:
-        return httmock.response(requests.codes.INTERNAL_SERVER_ERROR)
+    tunnel = url.path.split('/')[-1]
+    ipsec_policy_id = tunnel[6:]
+    content = {u'kind': u'object#vpn-site-to-site',
+               u'vpn-interface-name': u'%s' % tunnel,
+               u'ip-version': u'ipv4',
+               u'vpn-type': u'site-to-site',
+               u'ipsec-policy-id': u'%s' % ipsec_policy_id,
+               u'ike-profile-id': None,
+               u'mtu': 1500,
+               u'local-device': {
+                   u'ip-address': u'GigabitEthernet3',
+                   u'tunnel-ip-address': u'10.10.10.10'
+               },
+               u'remote-device': {
+                   u'tunnel-ip-address': u'10.10.10.20'
+               }}
+    return httmock.response(requests.codes.OK, content=content)
 
 
 @filter_request(['get'], 'vpn-svc/site-to-site')

diff --git a/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py b/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py
index 920b9e403bd364ef143f6a41f45dac602156015a..776d4da464d284ebdddcd64fc7e3a0d35fbcdead 100644 (file)
--- a/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py
+++ b/neutron/tests/unit/services/vpn/device_drivers/notest_cisco_csr_rest.py
@@ -919,17 +919,13 @@ class TestCsrRestIPSecConnectionCreate(base.BaseTestCase):
                           location)
             # Check the hard-coded items that get set as well...
             content = self.csr.get_request(location, full_url=True)
-            if csr_request.FIXED_CSCum50512:
-                self.assertEqual(requests.codes.OK, self.csr.status)
-                expected_connection = {u'kind': u'object#vpn-site-to-site',
-                                       u'ip-version': u'ipv4'}
-                expected_connection.update(connection_info)
-                expected_connection[u'local-device'][u'ip-address'] = (
-                    u'unnumbered GigabitEthernet3')
-                self.assertEqual(expected_connection, content)
-            else:
-                self.assertEqual(requests.codes.INTERNAL_SERVER_ERROR,
-                                 self.csr.status)
+            self.assertEqual(requests.codes.OK, self.csr.status)
+            expected_connection = {u'kind': u'object#vpn-site-to-site',
+                                   u'ike-profile-id': None,
+                                   u'mtu': 1500,
+                                   u'ip-version': u'ipv4'}
+            expected_connection.update(connection_info)
+            self.assertEqual(expected_connection, content)
 
     def test_create_ipsec_connection_no_pre_shared_key(self):
         """Test of connection create without associated pre-shared key.
@@ -1036,6 +1032,9 @@ class TestCsrRestIPSecConnectionCreate(base.BaseTestCase):
                 u'remote-device': {u'tunnel-ip-address': u'10.10.10.20'}
             }
             self.csr.create_ipsec_connection(connection_info)
+            self.addCleanup(self._remove_resource_for_test,
+                            self.csr.delete_ipsec_connection,
+                            'Tunnel%d' % tunnel_id)
             self.assertEqual(requests.codes.BAD_REQUEST, self.csr.status)
 
     def test_create_ipsec_connection_with_max_mtu(self):
@@ -1080,6 +1079,9 @@ class TestCsrRestIPSecConnectionCreate(base.BaseTestCase):
                 u'remote-device': {u'tunnel-ip-address': u'10.10.10.20'}
             }
             self.csr.create_ipsec_connection(connection_info)
+            self.addCleanup(self._remove_resource_for_test,
+                            self.csr.delete_ipsec_connection,
+                            'Tunnel%d' % tunnel_id)
             self.assertEqual(requests.codes.BAD_REQUEST, self.csr.status)
 
     def test_status_when_no_tunnels_exist(self):

diff --git a/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py b/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py
index d2ff8796e951c4ddb1887e96776fd3448366f5bd..78a0bd84fa2fb70bf98e8f7ab70159cb0cd5eba5 100644 (file)
--- a/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py
+++ b/neutron/tests/unit/services/vpn/device_drivers/test_cisco_ipsec.py
@@ -426,7 +426,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase):
         self.assertFalse(connection.is_dirty)
         self.assertEqual(u'Tunnel0', connection.tunnel)
         self.assertEqual(constants.ACTIVE, connection.last_status)
-        self.assertEqual(0, self.conn_create.call_count)
+        self.assertFalse(self.conn_create.called)
         # TODO(pcm) FUTURE - handling for update (delete/create?)
 
     def test_update_of_unknown_ipsec_connection(self):
@@ -468,7 +468,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase):
         self.assertFalse(connection.is_dirty)
         self.assertEqual(u'Tunnel0', connection.tunnel)
         self.assertEqual(constants.ACTIVE, connection.last_status)
-        self.assertEqual(0, self.conn_create.call_count)
+        self.assertFalse(self.conn_create.called)
 
     def test_update_connection_admin_down(self):
         """Connection updated to admin down state - dirty."""
@@ -488,7 +488,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase):
         self.assertTrue(connection.is_dirty)
         self.assertEqual(u'Tunnel0', connection.tunnel)
         self.assertEqual(constants.DOWN, connection.last_status)
-        self.assertEqual(0, self.conn_create.call_count)
+        self.assertFalse(self.conn_create.called)
 
     def test_update_missing_connection_admin_down(self):
         """Connection not present is in admin down state - nop.
@@ -506,7 +506,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase):
         connection = self.driver.update_connection(self.context,
                                                    u'123', conn_data)
         self.assertIsNone(connection)
-        self.assertEqual(0, self.conn_create.call_count)
+        self.assertFalse(self.conn_create.called)
 
     def test_update_for_vpn_service_create(self):
         """Creation of new IPSec connection on new VPN service - create.
@@ -586,7 +586,7 @@ class TestCiscoCsrIPsecDeviceDriverSyncStatuses(base.BaseTestCase):
         self.assertFalse(connection.is_dirty)
         self.assertEqual(u'Tunnel0', connection.tunnel)
         self.assertEqual(constants.ACTIVE, connection.last_status)
-        self.assertEqual(0, self.conn_create.call_count)
+        self.assertFalse(self.conn_create.called)
 
     def test_update_service_admin_down(self):
         """VPN service updated to admin down state - dirty.

diff --git a/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py b/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py
index 7c53e6fa7a6a05e3cec52e6edf79ca62db6d5b0e..50f3bc2c335fa44c15202df08d5a256d4c901a10 100644 (file)
--- a/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py
+++ b/neutron/tests/unit/services/vpn/service_drivers/test_cisco_ipsec.py
@@ -309,12 +309,12 @@ class TestCiscoIPsecDriver(base.BaseTestCase):
         mock.patch.object(csr_db, 'create_tunnel_mapping').start()
         self.context = n_ctx.Context('some_user', 'some_tenant')
 
-    def _test_update(self, func, args):
+    def _test_update(self, func, args, reason=None):
         with mock.patch.object(self.driver.agent_rpc, 'cast') as cast:
             func(self.context, *args)
             cast.assert_called_once_with(
                 self.context,
-                {'args': {},
+                {'args': reason,
                  'namespace': None,
                  'method': 'vpnservice_updated'},
                 version='1.0',
@@ -322,7 +322,8 @@ class TestCiscoIPsecDriver(base.BaseTestCase):
 
     def test_create_ipsec_site_connection(self):
         self._test_update(self.driver.create_ipsec_site_connection,
-                          [FAKE_VPN_CONNECTION])
+                          [FAKE_VPN_CONNECTION],
+                          {'reason': 'ipsec-conn-create'})
 
     def test_failure_validation_ipsec_connection(self):
         """Failure test of validation during IPSec site connection create.
@@ -352,12 +353,15 @@ class TestCiscoIPsecDriver(base.BaseTestCase):
 
     def test_delete_ipsec_site_connection(self):
         self._test_update(self.driver.delete_ipsec_site_connection,
-                          [FAKE_VPN_CONNECTION])
+                          [FAKE_VPN_CONNECTION],
+                          {'reason': 'ipsec-conn-delete'})
 
     def test_update_vpnservice(self):
         self._test_update(self.driver.update_vpnservice,
-                          [FAKE_VPN_SERVICE, FAKE_VPN_SERVICE])
+                          [FAKE_VPN_SERVICE, FAKE_VPN_SERVICE],
+                          {'reason': 'vpn-service-update'})
 
     def test_delete_vpnservice(self):
         self._test_update(self.driver.delete_vpnservice,
-                          [FAKE_VPN_SERVICE])
+                          [FAKE_VPN_SERVICE],
+                          {'reason': 'vpn-service-delete'})