Create and use a Adapter wrapper for REST in TaskManager

Adding a TaskManager aware wrapper around Adapter gets us a natural
interface for migrating one call at a time away from python-*client and
to direct REST calls. The Adapter shares the Session with the clients,
but it returns munches and throws exceptions that match the exceptions
we're expecting outside of shade.

Also, move compute_client calls out of their own tasks

Putting compute_client calls in a Task will cause them to be doubly
enqueued. This has to be combined with this patch because otherwise the
double-invocation fails functional tests.

Change-Id: If2d42af5fde1334b3b99ec3a9bbade38b19adbee

requirements.txt

@@ -5,7 +5,7 @@ decorator
 jmespath
 jsonpatch
 ipaddress
-os-client-config>=1.20.0
+os-client-config>=1.22.0
 requestsexceptions>=1.1.1
 six
 

shade/_adapter.py

@@ -0,0 +1,163 @@
+# Copyright (c) 2016 Red Hat, Inc.
+#
+# 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.
+
+''' Wrapper around keystoneauth Session to wrap calls in TaskManager '''
+
+import functools
+from keystoneauth1 import adapter
+from six.moves import urllib
+
+from shade import exc
+from shade import meta
+from shade import task_manager
+
+
+def extract_name(url):
+    '''Produce a key name to use in logging/metrics from the URL path.
+
+    We want to be able to logic/metric sane general things, so we pull
+    the url apart to generate names. The function returns a list because
+    there are two different ways in which the elements want to be combined
+    below (one for logging, one for statsd)
+
+    Some examples are likely useful:
+
+    /servers -> ['servers']
+    /servers/{id} -> ['servers']
+    /servers/{id}/os-security-groups -> ['servers', 'os-security-groups']
+    /v2.0/networks.json -> ['networks']
+    '''
+
+    url_path = urllib.parse.urlparse(url).path.strip()
+    # Remove / from the beginning to keep the list indexes of interesting
+    # things consistent
+    if url_path.startswith('/'):
+        url_path = url_path[1:]
+
+    # Special case for neutron, which puts .json on the end of urls
+    if url_path.endswith('.json'):
+        url_path = url_path[:-len('.json')]
+
+    url_parts = url_path.split('/')
+    if url_parts[-1] == 'detail':
+        # Special case detail calls
+        # GET /servers/detail
+        # returns ['servers', 'detail']
+        name_parts = url_parts[-2:]
+    else:
+        # Strip leading version piece so that
+        # GET /v2.0/networks
+        # returns ['networks']
+        if url_parts[0] in ('v1', 'v2', 'v2.0'):
+            url_parts = url_parts[1:]
+        name_parts = []
+        # Pull out every other URL portion - so that
+        # GET /servers/{id}/os-security-groups
+        # returns ['servers', 'os-security-groups']
+        for idx in range(0, len(url_parts)):
+            if not idx % 2 and url_parts[idx]:
+                name_parts.append(url_parts[idx])
+
+    # Keystone Token fetching is a special case, so we name it "tokens"
+    if url_path.endswith('tokens'):
+        name_parts = ['tokens']
+
+    # Getting the root of an endpoint is doing version discovery
+    if not name_parts:
+        name_parts = ['discovery']
+
+    # Strip out anything that's empty or None
+    return [part for part in name_parts if part]
+
+
+class ShadeAdapter(adapter.Adapter):
+
+    def __init__(self, shade_logger, manager, *args, **kwargs):
+        super(ShadeAdapter, self).__init__(*args, **kwargs)
+        self.shade_logger = shade_logger
+        self.manager = manager
+
+    def _munch_response(self, response, result_key=None):
+        exc.raise_from_response(response)
+        # Glance image downloads just return the data in the body
+        if response.headers.get('Content-Type') == 'application/octet-stream':
+            return response
+        else:
+            if not response.content:
+                # This doens't have any content
+                return response
+            try:
+                result_json = response.json()
+            except Exception:
+                self.shade_logger.debug(
+                    "Problems decoding json from response."
+                    " Reponse: {code} {reason}".format(
+                        code=response.status_code,
+                        reason=response.reason))
+                raise
+
+            # Wrap the keys() call in list() because in python3 keys returns
+            # a "dict_keys" iterator-like object rather than a list
+            json_keys = list(result_json.keys())
+            if len(json_keys) > 1 and result_key:
+                result = result_json[result_key]
+            elif len(json_keys) == 1:
+                result = result_json[json_keys[0]]
+            else:
+                # Yay for inferrence!
+                path = urllib.parse.urlparse(response.url).path.strip()
+                object_type = path.split('/')[-1]
+                if object_type in json_keys:
+                    result = result_json[object_type]
+                elif (object_type.startswith('os-')
+                      and object_type[3:] in json_keys):
+                    result = result_json[object_type[3:]]
+                else:
+                    raise exc.OpenStackCloudException(
+                        "Cannot find the resource value in the returned json."
+                        " This is a bug in shade. Please report it.")
+
+        request_id = response.headers.get('x-openstack-request-id')
+
+        if task_manager._is_listlike(result):
+            return meta.obj_list_to_dict(result, request_id=request_id)
+        elif task_manager._is_objlike(result):
+            return meta.obj_to_dict(result, request_id=request_id)
+        return result
+
+    def request(self, url, method, *args, **kwargs):
+        service_type = kwargs.get(
+            'endpoint_filter', {}).get('service_type', 'auth')
+
+        name_parts = extract_name(url)
+        name = '.'.join([service_type, method] + name_parts)
+        class_name = "".join([
+            part.lower().capitalize() for part in name.split('.')])
+
+        request_method = functools.partial(
+            super(ShadeAdapter, self).request, url, method)
+
+        class RequestTask(task_manager.BaseTask):
+
+            def __init__(self, **kw):
+                super(RequestTask, self).__init__(**kw)
+                self.name = name
+                self.__class__.__name__ = str(class_name)
+
+            def main(self, client):
+                self.args.setdefault('raise_exc', False)
+                return request_method(**self.args)
+
+        response = self.manager.submit_task(RequestTask(**kwargs))
+        return self._munch_response(response)

shade/_tasks.py

@@ -87,32 +87,6 @@ class FlavorList(task_manager.Task):
         return client.nova_client.flavors.list(**self.args)
 
 
-class FlavorGetExtraSpecs(task_manager.RequestTask):
-    result_key = 'extra_specs'
-
-    def main(self, client):
-        return client._compute_client.get(
-            "/flavors/{id}/os-extra_specs".format(**self.args))
-
-
-class FlavorSetExtraSpecs(task_manager.RequestTask):
-    result_key = 'extra_specs'
-
-    def main(self, client):
-        return client._compute_client.post(
-            "/flavors/{id}/os-extra_specs".format(**self.args),
-            json=self.args['json']
-            )
-
-
-class FlavorUnsetExtraSpecs(task_manager.RequestTask):
-
-    def main(self, client):
-        return client._compute_client.delete(
-            "/flavors/{id}/os-extra_specs/{key}".format(**self.args),
-            )
-
-
 class FlavorCreate(task_manager.Task):
     def main(self, client):
         return client.nova_client.flavors.create(**self.args)
@@ -267,18 +241,6 @@ class KeypairDelete(task_manager.Task):
         return client.nova_client.keypairs.delete(**self.args)
 
 
-class NovaListExtensions(task_manager.RequestTask):
-    result_key = 'extensions'
-
-    def main(self, client):
-        return client._compute_client.get('/extensions')
-
-
-class NovaUrlGet(task_manager.RequestTask):
-    def main(self, client):
-        return client._compute_client.get(**self.args)
-
-
 class NetworkList(task_manager.Task):
     def main(self, client):
         return client.neutron_client.list_networks(**self.args)

shade/exc.py

@@ -63,9 +63,37 @@ class OpenStackCloudUnavailableFeature(OpenStackCloudException):
     pass
 
 
-class OpenStackCloudResourceNotFound(OpenStackCloudException):
-    pass
+class OpenStackCloudHTTPError(OpenStackCloudException):
+
+    def __init__(self, message, response=None):
+        super(OpenStackCloudHTTPError, self).__init__(message)
+        self.response = response
 
 
-class OpenStackCloudURINotFound(OpenStackCloudException):
+class OpenStackCloudURINotFound(OpenStackCloudHTTPError):
     pass
+
+# Backwards compat
+OpenStackCloudResourceNotFound = OpenStackCloudURINotFound
+
+
+# Logic shamelessly stolen from requests
+def raise_from_response(response):
+    msg = ''
+    if 400 <= response.status_code < 500:
+        msg = '({code}) Client Error: {reason} for url: {url}'.format(
+            code=response.status_code,
+            reason=response.reason,
+            url=response.url)
+    elif 500 <= response.status_code < 600:
+        msg = '({code}) Server Error: {reason} for url: {url}'.format(
+            code=response.status_code,
+            reason=response.reason,
+            url=response.url)
+
+    # Special case 404 since we raised a specific one for neutron exceptions
+    # before
+    if response.status_code == 404:
+        raise OpenStackCloudURINotFound(msg, response=response)
+    if msg:
+        raise OpenStackCloudHTTPError(msg, response=response)

shade/openstackcloud.py

@@ -47,6 +47,7 @@ import troveclient.client
 import designateclient.client
 
 from shade.exc import *  # noqa
+from shade import _adapter
 from shade import _log
 from shade import _normalize
 from shade import meta
@@ -142,8 +143,10 @@ class OpenStackCloud(_normalize.Normalizer):
             OpenStackCloudException.log_inner_exceptions = True
 
         self.log = _log.setup_logging('shade')
+
         if not cloud_config:
             config = os_client_config.OpenStackConfig()
+
             cloud_config = config.get_one_cloud(**kwargs)
 
         self.name = cloud_config.name
@@ -157,10 +160,17 @@ class OpenStackCloud(_normalize.Normalizer):
         self.force_ipv4 = cloud_config.force_ipv4
         self.strict_mode = strict
 
+        if manager is not None:
+            self.manager = manager
+        else:
+            self.manager = task_manager.TaskManager(
+                name=':'.join([self.name, self.region_name]), client=self)
+
         # Provide better error message for people with stale OCC
-        if cloud_config.get_external_ipv4_networks is None:
+        if cloud_config.set_session_constructor is None:
             raise OpenStackCloudException(
-                "shade requires at least version 1.20.0 of os-client-config")
+                "shade requires at least version 1.22.0 of os-client-config")
+
         self._external_ipv4_names = cloud_config.get_external_ipv4_networks()
         self._internal_ipv4_names = cloud_config.get_internal_ipv4_networks()
         self._external_ipv6_names = cloud_config.get_external_ipv6_networks()
@@ -181,12 +191,6 @@ class OpenStackCloud(_normalize.Normalizer):
         self._use_internal_network = cloud_config.config.get(
             'use_internal_network', True)
 
-        if manager is not None:
-            self.manager = manager
-        else:
-            self.manager = task_manager.TaskManager(
-                name=':'.join([self.name, self.region_name]), client=self)
-
         # Work around older TaskManager objects that don't have submit_task
         if not hasattr(self.manager, 'submit_task'):
             self.manager.submit_task = self.manager.submitTask
@@ -365,7 +369,14 @@ class OpenStackCloud(_normalize.Normalizer):
         return client
 
     def _get_raw_client(self, service_key):
-        return self.cloud_config.get_session_client(service_key)
+        return _adapter.ShadeAdapter(
+            manager=self.manager,
+            session=self.cloud_config.get_session(),
+            service_type=self.cloud_config.get_service_type(service_key),
+            service_name=self.cloud_config.get_service_name(service_key),
+            interface=self.cloud_config.get_interface(service_key),
+            region_name=self.cloud_config.region,
+            shade_logger=self.log)
 
     @property
     def _compute_client(self):
@@ -1260,8 +1271,7 @@ class OpenStackCloud(_normalize.Normalizer):
         extensions = set()
 
         with _utils.shade_exceptions("Error fetching extension list for nova"):
-            for extension in self.manager.submit_task(
-                    _tasks.NovaListExtensions()):
+            for extension in self._compute_client.get('/extensions'):
                 extensions.add(extension['alias'])
 
         return extensions
@@ -1536,10 +1546,11 @@ class OpenStackCloud(_normalize.Normalizer):
         with _utils.shade_exceptions("Error fetching flavor extra specs"):
             for flavor in flavors:
                 if not flavor.extra_specs and get_extra:
+                    endpoint = "/flavors/{id}/os-extra_specs".format(
+                        id=flavor.id)
                     try:
-                        flavor.extra_specs = self.manager.submit_task(
-                            _tasks.FlavorGetExtraSpecs(id=flavor.id))
-                    except keystoneauth1.exceptions.http.HttpError as e:
+                        flavor.extra_specs = self._compute_client.get(endpoint)
+                    except OpenStackCloudHttpError as e:
                         flavor.extra_specs = []
                         self.log.debug(
                             'Fetching extra specs for flavor failed:'

shade/operatorcloud.py

@@ -1526,9 +1526,9 @@ class OperatorCloud(openstackcloud.OpenStackCloud):
         :raises: OpenStackCloudResourceNotFound if flavor ID is not found.
         """
         try:
-            self.manager.submit_task(
-                _tasks.FlavorSetExtraSpecs(
-                    id=flavor_id, json=dict(extra_specs=extra_specs)))
+            self._compute_client.post(
+                "/flavors/{id}/os-extra_specs".format(id=flavor_id),
+                json=dict(extra_specs=extra_specs))
         except Exception as e:
             raise OpenStackCloudException(
                 "Unable to set flavor specs: {0}".format(str(e))
@@ -1545,8 +1545,9 @@ class OperatorCloud(openstackcloud.OpenStackCloud):
         """
         for key in keys:
             try:
-                self.manager.submit_task(
-                    _tasks.FlavorUnsetExtraSpecs(id=flavor_id, key=key))
+                self._compute_client.delete(
+                    "/flavors/{id}/os-extra_specs/{key}".format(
+                        id=flavor_id, key=key))
             except Exception as e:
                 raise OpenStackCloudException(
                     "Unable to delete flavor spec {0}: {1}".format(

shade/tests/unit/test__adapter.py

@@ -0,0 +1,40 @@
+# -*- coding: utf-8 -*-
+
+# 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 testscenarios import load_tests_apply_scenarios as load_tests  # noqa
+
+from shade import _adapter
+from shade.tests.unit import base
+
+
+class TestExtractName(base.TestCase):
+
+    scenarios = [
+        ('slash_servers_bare', dict(url='/servers', parts=['servers'])),
+        ('slash_servers_arg', dict(url='/servers/1', parts=['servers'])),
+        ('servers_bare', dict(url='servers', parts=['servers'])),
+        ('servers_arg', dict(url='servers/1', parts=['servers'])),
+        ('networks_bare', dict(url='/v2.0/networks', parts=['networks'])),
+        ('networks_arg', dict(url='/v2.0/networks/1', parts=['networks'])),
+        ('tokens', dict(url='/v3/tokens', parts=['tokens'])),
+        ('discovery', dict(url='/', parts=['discovery'])),
+        ('secgroups', dict(
+            url='/servers/1/os-security-groups',
+            parts=['servers', 'os-security-groups'])),
+    ]
+
+    def test_extract_name(self):
+
+        results = _adapter.extract_name(self.url)
+        self.assertEqual(self.parts, results)

shade/tests/unit/test_caching.py

@@ -317,12 +317,10 @@ class TestMemoryCache(base.TestCase):
     @mock.patch.object(shade.OpenStackCloud, '_compute_client')
     @mock.patch.object(shade.OpenStackCloud, 'nova_client')
     def test_list_flavors(self, nova_mock, mock_compute):
+        # TODO(mordred) Change this to request_mock
         nova_mock.flavors.list.return_value = []
         nova_mock.flavors.api.client.get.return_value = {}
-        mock_response = mock.Mock()
-        mock_response.json.return_value = dict(extra_specs={})
-        mock_response.headers.get.return_value = 'request-id'
-        mock_compute.get.return_value = mock_response
+        mock_compute.get.return_value = {}
         self.assertEqual([], self.cloud.list_flavors())
 
         fake_flavor = fakes.FakeFlavor('555', 'vanilla', 100)

shade/tests/unit/test_shade.py

@@ -686,29 +686,25 @@ class TestShade(base.TestCase):
 
     @mock.patch.object(shade.OpenStackCloud, '_compute_client')
     def test__nova_extensions(self, mock_compute):
-        body = {
-            'extensions': [
-                {
-                    "updated": "2014-12-03T00:00:00Z",
-                    "name": "Multinic",
-                    "links": [],
-                    "namespace": "http://openstack.org/compute/ext/fake_xml",
-                    "alias": "NMN",
-                    "description": "Multiple network support."
-                },
-                {
-                    "updated": "2014-12-03T00:00:00Z",
-                    "name": "DiskConfig",
-                    "links": [],
-                    "namespace": "http://openstack.org/compute/ext/fake_xml",
-                    "alias": "OS-DCF",
-                    "description": "Disk Management Extension."
-                },
-            ]
-        }
-        mock_response = mock.Mock()
-        mock_response.json.return_value = body
-        mock_compute.get.return_value = mock_response
+        body = [
+            {
+                "updated": "2014-12-03T00:00:00Z",
+                "name": "Multinic",
+                "links": [],
+                "namespace": "http://openstack.org/compute/ext/fake_xml",
+                "alias": "NMN",
+                "description": "Multiple network support."
+            },
+            {
+                "updated": "2014-12-03T00:00:00Z",
+                "name": "DiskConfig",
+                "links": [],
+                "namespace": "http://openstack.org/compute/ext/fake_xml",
+                "alias": "OS-DCF",
+                "description": "Disk Management Extension."
+            },
+        ]
+        mock_compute.get.return_value = body
         extensions = self.cloud._nova_extensions()
         mock_compute.get.assert_called_once_with('/extensions')
         self.assertEqual(set(['NMN', 'OS-DCF']), extensions)
@@ -724,29 +720,25 @@ class TestShade(base.TestCase):
 
     @mock.patch.object(shade.OpenStackCloud, '_compute_client')
     def test__has_nova_extension(self, mock_compute):
-        body = {
-            'extensions': [
-                {
-                    "updated": "2014-12-03T00:00:00Z",
-                    "name": "Multinic",
-                    "links": [],
-                    "namespace": "http://openstack.org/compute/ext/fake_xml",
-                    "alias": "NMN",
-                    "description": "Multiple network support."
-                },
-                {
-                    "updated": "2014-12-03T00:00:00Z",
-                    "name": "DiskConfig",
-                    "links": [],
-                    "namespace": "http://openstack.org/compute/ext/fake_xml",
-                    "alias": "OS-DCF",
-                    "description": "Disk Management Extension."
-                },
-            ]
-        }
-        mock_response = mock.Mock()
-        mock_response.json.return_value = body
-        mock_compute.get.return_value = mock_response
+        body = [
+            {
+                "updated": "2014-12-03T00:00:00Z",
+                "name": "Multinic",
+                "links": [],
+                "namespace": "http://openstack.org/compute/ext/fake_xml",
+                "alias": "NMN",
+                "description": "Multiple network support."
+            },
+            {
+                "updated": "2014-12-03T00:00:00Z",
+                "name": "DiskConfig",
+                "links": [],
+                "namespace": "http://openstack.org/compute/ext/fake_xml",
+                "alias": "OS-DCF",
+                "description": "Disk Management Extension."
+            },
+        ]
+        mock_compute.get.return_value = body
         self.assertTrue(self.cloud._has_nova_extension('NMN'))
         self.assertFalse(self.cloud._has_nova_extension('invalid'))