Merge lp:~rconradharris/glance/lp615670 into lp:glance/austin

[Rick Clark]

First cut of backends and image controller.

First cut of backends and image controller. Not much here, just getting the ball rolling.

[Cory Wright (corywright)]

review: Approve

[Rick Clark (dendrobates)]

Framework looks good

review: Approve (code)

[Michael Gundlach (gundlach)]

Overall, looks good. Some style stuff and a couple of small but real complaints:

If this project is supposed to adhere to the same style guidelines as Nova, give every class and every method a docstring. E.g. it took me a while to get started understanding the parallax backend code because there was nothing to guide me, and I still don't know what the ImageRegistry docstring means.

In a similar vein, you should have a blank line between class line and docstring and 1st function, like

class Foo(object):

    """docstring"""

    def f1(self):
        """docstring"""
        ...

    ...

Sign your TODOs, like TODO(gundlach): so they aren't unclaimed.

You've got duplicate code in the while loops that yield CHUNKSIZE chunks. I'd move that duplicated code into Backend (which makes sense since Backend defined CHUNKSIZE -- it already understands that things will be read in chunks, so a generator that yields chunks would make sense living on Backend.)

Your try/except KeyError has too much in the try statement. Call _scheme2backend in the try only, then outside the try/except call get(). This way a KeyError in get() won't be misunderstood. It'll make the code look uglier, which hurts (I like how it looks now, visually) but it'll be less prone to bugs.

I thought the **kwargs passed to get_from_backend were YAGNI, but I see they are used for testing -- brilliant :)

If Glance is supposed to adhere to the same pylint rules as Nova, you're going to get flagged for PPRINT_OBJ unless you pylint: disable-msg the relevant rule. I don't personally care since we aren't enforcing pylint yet, but FYI.

Also FYI -- no action required if you don't want -- jaypipes(I think?) is making an openstack.common project for code common to nova/swift/glance/etc. One thing that will live in there is nova.wsgi.Router, which would help tease apart ImageController's two tasks (routing, and handling the requested action). You might copy nova.wsgi.Router into your project until openstack.common is ready, if you like. I'd be happy to help if you'd like handholding. Finally, if you haven't seen the webob.dec.wsgify decorator, check it out -- it is great for making WSGI apps look even simpler. It lets __call__ return Response objects directly, or strings, or webob.exc exceptions; and it eliminates start_response. With wsgi.Router and wsgify combined you'd have something like

class Router(wsgi.Router):
  def __init__(self):
    controller = ImageController()
    mapper = routes.Mapper()
    mapper.connect('/healthcheck', controller=controller, action='healthcheck')
    mapper.connect('/', controller=controller, action='fetch', conditions=dict(method="GET"))
    mapper.connect('/', controller=controller, action='put', conditions=dict(method="PUT"))
    mapper.connect('/', controller=controller, action='delete', conditions=dict(method="DELETE"))
    super(Router, self).__init__(mapper)

class ImageController(object):
  @webob.dec.wsgify
  def __call__(self, req):
    action = req.environ['wsgiorg.routing_args'][1]['action']
    return getattr(self, action)(req)
  def healthcheck(self, req):
    # healthcheck
  def fetch(self, req):
    # as before, but ends with "...

review: Needs Fixing

[Rick Harris (rconradharris)]

Thanks for the comments!
> Overall, looks good. Some style stuff and a couple of small but real
> complaints:
>
>
> If this project is supposed to adhere to the same style guidelines as Nova,
> give every class and every method a docstring. E.g. it took me a while to get
> started understanding the parallax backend code because there was nothing to
> guide me, and I still don't know what the ImageRegistry docstring means.
>
> In a similar vein, you should have a blank line between class line and
> docstring and 1st function, like
>
> class Foo(object):
>
> """docstring"""
>
> def f1(self):
> """docstring"""
> ...
>
> ...
>
>
> Sign your TODOs, like TODO(gundlach): so they aren't unclaimed.

I think glance should follow the nova coding standards (for better or worse), so I agree with all of these suggestions.

> You've got duplicate code in the while loops that yield CHUNKSIZE chunks. I'd
> move that duplicated code into Backend (which makes sense since Backend
> defined CHUNKSIZE -- it already understands that things will be read in
> chunks, so a generator that yields chunks would make sense living on Backend.)

You're right, the code isn't particularly DRY. Wrapping a file-like object in a generator is a common pattern and merits its own helper-function.

> Your try/except KeyError has too much in the try statement. Call
> _scheme2backend in the try only, then outside the try/except call get(). This
> way a KeyError in get() won't be misunderstood. It'll make the code look
> uglier, which hurts (I like how it looks now, visually) but it'll be less
> prone to bugs.

You are 100% right here, this is bad form.

> I thought the **kwargs passed to get_from_backend were YAGNI, but I see they
> are used for testing -- brilliant :)
>
> If Glance is supposed to adhere to the same pylint rules as Nova, you're going
> to get flagged for PPRINT_OBJ unless you pylint: disable-msg the relevant
> rule. I don't personally care since we aren't enforcing pylint yet, but FYI.

Agree, should probably make this pylint compliant (or ax it entirely).

> Also FYI -- no action required if you don't want -- jaypipes(I think?) is
> making an openstack.common project for code common to nova/swift/glance/etc.
> One thing that will live in there is nova.wsgi.Router, which would help tease
> apart ImageController's two tasks (routing, and handling the requested
> action). You might copy nova.wsgi.Router into your project until
> openstack.common is ready, if you like. I'd be happy to help if you'd like
> handholding. Finally, if you haven't seen the webob.dec.wsgify decorator,
> check it out -- it is great for making WSGI apps look even simpler. It lets
> __call__ return Response objects directly, or strings, or webob.exc
> exceptions; and it eliminates start_response. With wsgi.Router and wsgify
> combined you'd have something like
>
> class Router(wsgi.Router):
> def __init__(self):
> controller = ImageController()
> mapper = routes.Mapper()
> mapper.connect('/healthcheck', controller=controller,
> action='healthcheck')
> mapper.connect('/', controller=controller, action='fetch',
> conditions=dict(met...

=== added file '.bzrignore'
--- .bzrignore	1970-01-01 00:00:00 +0000
+++ .bzrignore	2010-09-08 21:07:40 +0000
@@ -0,0 +1,1 @@
+backends.pyc

=== modified file 'parallax/parallax/__init__.py'
--- parallax/parallax/__init__.py	2010-08-06 03:06:42 +0000
+++ parallax/parallax/__init__.py	2010-09-08 21:07:40 +0000
@@ -0,0 +1,16 @@
+class ImageRegistry(object):
+    """
+    {
+        "uuid": uuid,
+        "slug": slug,
+        "name": name,
+        "objects": [
+            { "uri": obj1_uri },
+            { "uri": obj2_uri }
+        ]
+    }
+    """
+    def __init__(self):
+        pass
+
+

=== added file 'teller/teller/backends.py'
--- teller/teller/backends.py	1970-01-01 00:00:00 +0000
+++ teller/teller/backends.py	2010-09-08 21:07:40 +0000
@@ -0,0 +1,80 @@
+import urlparse
+
+class BackendException(Exception):
+    pass
+class UnsupportedBackend(BackendException):
+    pass
+
+class Backend(object):
+    CHUNKSIZE = 4096
+
+class TestStrBackend(Backend):
+    @classmethod
+    def get(cls, parsed_uri):
+        """
+        teststr://data
+        """
+        yield parsed_uri.netloc
+
+class FilesystemBackend(Backend):
+    @classmethod
+    def get(cls, parsed_uri, opener=lambda p: open(p, "b")):
+        """
+        file:///path/to/file.tar.gz.0
+        """
+        def sanitize_path(p):
+            #FIXME: must prevent attacks using ".." and "." paths
+            return p
+
+        with opener(sanitize_path(parsed_uri.path)) as f:
+            chunk = f.read(cls.CHUNKSIZE)
+            while chunk:
+                yield chunk
+                chunk = f.read(cls.CHUNKSIZE)
+         
+class HTTPBackend(Backend):
+    @classmethod
+    def get(cls, parsed_uri, conn_class=None):
+        """
+        http://netloc/path/to/file.tar.gz.0
+        https://netloc/path/to/file.tar.gz.0
+        """
+        import httplib
+        if conn_class:
+            pass # use the conn_class passed in
+        elif parsed_uri.scheme == "http":
+            conn_class = httplib.HTTPConnection
+        elif parsed_uri.scheme == "https":
+            conn_class = httplib.HTTPSConnection
+        else:
+            raise BackendException("scheme '%s' not support for HTTPBackend")
+        conn = conn_class(parsed_uri.netloc)
+        conn.request("GET", parsed_uri.path, "", {})
+        try:
+            response = conn.getresponse()
+            chunk = response.read(cls.CHUNKSIZE)
+            while chunk:
+                yield chunk
+                chunk = response.read(cls.CHUNKSIZE)
+        finally:
+            conn.close()
+
+def _scheme2backend(scheme):
+    return {
+        "file": FilesystemBackend,
+        "http": HTTPBackend,
+        "https": HTTPBackend,
+        "teststr": TestStrBackend
+    }[scheme]
+
+def get_from_backend(uri, **kwargs):
+    """
+    Yields chunks of data from backend specified by uri
+    """
+    parsed_uri = urlparse.urlparse(uri)
+    try:
+        return _scheme2backend(parsed_uri.scheme).get(parsed_uri, **kwargs)
+    except KeyError:
+        raise UnsupportedBackend("No backend found for '%s'" % parsed_uri.scheme)
+
+

=== added file 'teller/teller/server.py'
--- teller/teller/server.py	1970-01-01 00:00:00 +0000
+++ teller/teller/server.py	2010-09-08 21:07:40 +0000
@@ -0,0 +1,76 @@
+from webob import Request, Response, UTC
+from webob.exc import HTTPAccepted, HTTPBadRequest, HTTPCreated, \
+    HTTPInternalServerError, HTTPNoContent, HTTPNotFound, \
+    HTTPNotModified, HTTPPreconditionFailed, \
+    HTTPRequestTimeout, HTTPUnprocessableEntity, HTTPMethodNotAllowed
+from teller.backends import get_from_backend
+
+def PPRINT_OBJ(obj):
+    from pprint import pprint
+    pprint(obj.__dict__)
+    print dir(obj)
+
+class ImageController(object):
+    """Implements the WSGI application for the Teller Image Server."""
+    def __init__(self, conf):
+        """
+        """
+        #TODO: add real lookup fn
+        #self.image_lookup_fn = parallax_lookup()
+        self.log_requests = conf.get('log_requests', 't')[:1].lower() == 't'
+
+    def GET(self, request):
+        try:
+            uri = request.GET['uri']
+        except KeyError:
+            return HTTPBadRequest(body="Missing uri", request=request,
+                                  content_type="text/plain")
+
+        image = self.image_lookup_fn(uri)
+        if not image:
+            return HTTPNotFound(body="Image not found", request=request,
+                                content_type="text/plain")
+
+        def image_iter():
+            for obj in image["objects"]:
+                for chunk in get_from_backend(obj["uri"]):
+                    yield chunk
+
+        return request.get_response(Response(app_iter=image_iter()))
+
+    def __call__(self, env, start_response):
+        """WSGI Application entry point for the Teller Image Server."""
+        start_time = time.time()
+        req = Request(env)
+        if False:
+            pass
+        #if req.path_info == '/healthcheck':
+        #    return healthcheck(req)(env, start_response)
+        #elif not check_xml_encodable(req.path_info):
+        #    res = HTTPPreconditionFailed(body='Invalid UTF8')
+        else:
+            try:
+                if hasattr(self, req.method):
+                    res = getattr(self, req.method)(req)
+                else:
+                    res = HTTPMethodNotAllowed()
+            except:
+                self.logger.exception('ERROR __call__ error with %s %s '
+                    % (env.get('REQUEST_METHOD', '-'), env.get('PATH_INFO', '-')))
+                res = HTTPInternalServerError(body=traceback.format_exc())
+        trans_time = time.time() - start_time
+        if self.log_requests:
+            log_line = '%s - - [%s] "%s %s" %s %s "%s" "%s" %.4f' % (
+                req.remote_addr,
+                time.strftime('%d/%b/%Y:%H:%M:%S +0000',
+                              time.gmtime()),
+                req.method, req.path, res.status.split()[0],
+                res.content_length or '-', req.referer or '-',
+                req.user_agent or '-',
+                trans_time)
+            self.logger.info(log_line)
+        #if req.method in ('PUT', 'DELETE'):
+        #    slow = self.slow - trans_time
+        #    if slow > 0:
+        #        sleep(slow)
+        return res(env, start_response)

=== added directory 'teller/tests/functional'
=== added directory 'teller/tests/unit'
=== added file 'teller/tests/unit/test_backends.py'
--- teller/tests/unit/test_backends.py	1970-01-01 00:00:00 +0000
+++ teller/tests/unit/test_backends.py	2010-09-08 21:07:40 +0000
@@ -0,0 +1,40 @@
+import unittest
+from StringIO import StringIO
+from teller.backends import Backend, get_from_backend
+
+class TestBackends(unittest.TestCase):
+    def setUp(self):
+        Backend.CHUNKSIZE = 2
+
+    def test_filesystem_get_from_backend(self):
+        class FakeFile(object):
+            def __enter__(self, *args, **kwargs):
+                return StringIO('fakedata')
+            def __exit__(self, *args, **kwargs):
+                pass
+
+        fetcher = get_from_backend("file:///path/to/file.tar.gz",
+                                   opener=lambda _: FakeFile())
+
+        chunks = [c for c in fetcher]
+        self.assertEqual(chunks, ["fa", "ke", "da", "ta"])
+
+    def test_http_get_from_backend(self):
+        class FakeHTTPConnection(object):
+            def __init__(self, *args, **kwargs):
+                pass
+            def request(self, *args, **kwargs):
+                pass
+            def getresponse(self):
+                return StringIO('fakedata')
+            def close(self):
+                pass
+
+        fetcher = get_from_backend("http://netloc/path/to/file.tar.gz",
+                                   conn_class=FakeHTTPConnection)
+
+        chunks = [c for c in fetcher]
+        self.assertEqual(chunks, ["fa", "ke", "da", "ta"])
+
+if __name__ == "__main__":
+    unittest.main()

=== added file 'teller/tests/unit/test_server.py'
--- teller/tests/unit/test_server.py	1970-01-01 00:00:00 +0000
+++ teller/tests/unit/test_server.py	2010-09-08 21:07:40 +0000
@@ -0,0 +1,37 @@
+import unittest
+from webob import Request
+#TODO: should this be teller.image ?
+from teller import server as image_server
+
+class TestImageController(unittest.TestCase):
+    def setUp(self):
+        conf = {}
+        self.image_controller = image_server.ImageController(conf)
+
+    def test_GET(self):
+        # uri must be specified
+        request = Request.blank("/image")
+        response = self.image_controller.GET(request)
+        self.assertEqual(response.status_int, 400) # should be 422?
+
+        # FIXME: need urllib.quote here?
+        image_uri = "http://parallax/myacct/my-image"
+        request = Request.blank("/image?uri=%s" % image_uri)
+        def mock_parallax_lookup(uri):
+            return {"objects": [{"uri": "teststr://chunk0"},
+                                {"uri": "teststr://chunk1"}]}
+
+        self.image_controller.image_lookup_fn = mock_parallax_lookup
+        response = self.image_controller.GET(request)
+        self.assertEqual("chunk0chunk1", response.body)
+
+        image_uri = "http://parallax/myacct/does-not-exist"
+        request = Request.blank("/image?uri=%s" % image_uri)
+        def mock_parallax_lookup(uri):
+            return None
+        self.image_controller.image_lookup_fn = mock_parallax_lookup
+        response = self.image_controller.GET(request)
+        self.assertEqual(response.status_int, 404)
+
+if __name__ == "__main__":
+    unittest.main()