migrate server-syncstorage to pyramid/cornice/mozsvc

RESOLVED FIXED

Status

Cloud Services
Server: Sync
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rfkelly, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa?])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 597745 [details] [diff] [review]
patch migrating server-storage to pyramid

The usual suspects for such a migration:
   * merge config files
   * replace wsgiapp.py with views.py
   * hook up new config handling

Some extra fun stuff:
   * needs a tween from mozscv to turn BackendError into "503 Service Unavailable"
   * needs accept-header handling from cornice trunk
(Reporter)

Comment 1

6 years ago
Created attachment 597746 [details] [diff] [review]
patch adding catch_backend_errors tween

additional patch required for mozsvc
Attachment #597746 - Flags: review?(telliott)
(Reporter)

Updated

6 years ago
Attachment #597745 - Flags: review?(telliott)
(Reporter)

Updated

6 years ago
Blocks: 727761
Attachment #597745 - Flags: review?(tarek)
Whiteboard: [qa?]
Attachment #597746 - Flags: review?(telliott) → review+
Comment on attachment 597745 [details] [diff] [review]
patch migrating server-storage to pyramid

looks globally good, I am - for the events part to make sure we have the best code there



>diff --git a/setup.py b/setup.py
>index c2a6e13..46cc972 100644
>--- a/setup.py
>+++ b/setup.py
>@@ -7,13 +7,13 @@ import re
> 
> 
> install_requires = ['SQLALchemy<=0.6.99', 'PasteDeploy', 'WebOb',
>-                    'Routes', 'simplejson', 'cef',
>-                    'Services>=1.0']

What about repoze.who, pyramid, pyramid_who and PyVep etc.. ?

a "python setup.py develop" should lead to a working storage package, at least for the tests

>+                    'Routes', 'simplejson', 'cef', 'mozsvc']
>+                    
> 
...


>+
>+def includeme(config):
>+    # Add config settings to use vepauth and basicauth.
>+    # This is a temporary hack.
>+    # It will go away once mozsvc gets built-in support for it, but is here
>+    # for now so that we can test the new auth flow.

Why can't we push those configuration values in the .ini file already ? 

 
>+    settings = config.registry.settings
>+    VEPAUTH_DEFAULTS = {
>+        "use": "repoze.who.plugins.vepauth:make_plugin",
>+        "audiences": "",
>+        "token_url": "/1.1/token",
>+        "token_manager": "syncstorage.tokens:ServicesTokenManager",
>+    }
>+    for key, value in VEPAUTH_DEFAULTS.iteritems():
>+        settings.setdefault("who.plugin.vepauth." + key, value)
>+    BASICAUTH_DEFAULTS = {
>+        "use": "repoze.who.plugins.basicauth:make_plugin",
>+        "realm": "Sync",
>+    }
>+    for key, value in BASICAUTH_DEFAULTS.iteritems():
>+        settings.setdefault("who.plugin.basicauth." + key, value)
>+    # Make sure there's a usable config for the "testauth" plugin.
>+    TESTAUTH_DEFAULTS = {
>+        "use": "syncstorage.tokens:TestingAuthenticator",
>+    }
>+    for key, value in TESTAUTH_DEFAULTS.iteritems():
>+        settings.setdefault("who.plugin.testauth." + key, value)
...


>+import itertools
> 
>-from webob.exc import HTTPBadRequest, HTTPNotFound, HTTPPreconditionFailed
>-#from cef import log_cef
>+from pyramid.httpexceptions import (HTTPBadRequest,
>+                                    HTTPNotFound,
>+                                    HTTPPreconditionFailed)
> 
>-from services.util import round_time, batch, HTTPJsonBadRequest
>-from services.formatters import convert_response, json_response
>-from services.respcodes import (WEAVE_MALFORMED_JSON, WEAVE_INVALID_WBO,
>-                                WEAVE_INVALID_WRITE, WEAVE_OVER_QUOTA)
>-from services import logger
>+from mozsvc.util import round_time
>+from mozsvc.exceptions import (ERROR_MALFORMED_JSON, ERROR_INVALID_WBO,
>+                               ERROR_INVALID_WRITE, ERROR_OVER_QUOTA)
>+
>+from syncstorage import logger
> from syncstorage.wbo import WBO
>+from syncstorage.storage import get_storage
> 
> 
> _WBO_FIELDS = ['id', 'parentid', 'predecessorid', 'sortindex', 'modified',
>@@ -25,21 +28,37 @@ _WBO_FIELDS = ['id', 'parentid', 'predecessorid', 'sortindex', 'modified',
> _ONE_MEG = 1024
> 
> 
>+def HTTPJsonBadRequest(data, **kwds):
>+    kwds.setdefault("content_type", "application/json")
>+    return HTTPBadRequest(body=json.dumps(data, use_decimal=True), **kwds)
>+

Nice ! (I prefer slices myself for readability but that's just a matter of taste :) ) http://code.activestate.com/recipes/303279-getting-items-in-batches/
Sound like this one could go in mozsvc.

>+def batch(iterable, size=100):
>+    """Returns the given iterable split into batches of the given size."""
>+    counter = itertools.count()
>+
>+    def ticker(key):
>+        return next(counter) // size
>+
>+    for key, group in itertools.groupby(iter(iterable), ticker):
>+        yield group
>+
>+

No need for parenthesis here, simply do:

  return used, limit


>     def get_quota(self, request):
>         user_id = request.user['userid']
>@@ -100,12 +104,12 @@ class StorageController(object):
>             limit = None
>         else:
>             limit = self._get_storage(request).quota_size
>-        return json_response((used, limit))
>+        return (used, limit)

...

I find INI_FILE a bit of a generic name for this. I would prefix it with SYNC_STPRAGE maybe

>+# the ini file is grabbed at its production place
>+# unless force via an environ variable
>+ini_file = os.path.join('/etc', 'syncserver', 'production.ini')
>+ini_file = os.path.abspath(os.environ.get('INI_FILE', ini_file))


What was wrong with the signals ? 

...

>@@ -37,9 +39,17 @@ class CacheManager(object):
>         # when several clients for the same user
>         # get/set the cached data
>         self._locker = threading.RLock()
>-        subscribe(REQUEST_ENDS, self._cleanup_pool)
>+        # clean up the pool when a request ends.

This seems complicated. Why aren't you using Pyramid events ? They work like the events I created in the micro-framework, but based on a @suscriber decorator

see http://docs.pylonsproject.org/projects/pyramid/en/1.0-branch/api/events.html#pyramid.events.subscriber


>+        # XXX: using threadlocal, yuck!  Need to find a
>+        # better way to do this.  Somehow give backends
>+        # access to the configurator?
>+        registry = get_current_registry()
>+        registry.registerHandler(self._setup_cleanup_pool, (NewRequest,))
>+
>+    def _setup_cleanup_pool(self, request):
>+        request.add_finished_callback(self._cleanup_pool)
> 
>-    def _cleanup_pool(self, response):
>+    def _cleanup_pool(self, request):
>         self.pool.pop(thread.get_ident(), None)
> 
>     def flush_all(self):
...


we can use urlparse here instead of splitting manually.

>+                assert value.split(':/')[0] in ('mysql', 'sqlite')
>+                self.sqlfiles.append(value.split('sqlite:///')[-1])
>+
>+        self.app = TestApp(self.config.make_wsgi_app())



Is there a way to configure tweens from the configuration file ?  
I am asking because this strikes me as we could have a lib of tweens in mozsvc we could reuse,

>+
>+def includeme(config):
>+    """Include all the SyncServer tweens into the given config."""
>+    config.add_tween("syncstorage.tweens.check_for_blacklisted_nodes")
>+    config.add_tween("syncstorage.tweens.set_x_timestamp_header")
>+    config.add_tween("syncstorage.tweens.set_default_accept_header")



>+
>+class WhoisiRenderer(object):
>+    """Pyramid renderer producing lists in application/whoisi format."""

why an empty constructor ? is this signature *required* ?

>+
>+    def __init__(self, info):
>+        pass
>+
>+    def __call__(self, value, system):
>+        request = system.get('request')
>+        if request is not None:
>+            response = request.response
>+            if response.content_type == response.default_content_type:
>+                response.content_type = "application/whoisi"
>+        data = []
>+        for line in value:
>+            line = json.dumps(line, use_decimal=True)
>+            size = struct.pack('!I', len(line))
>+            data.append('%s%s' % (size, line))
>+        return ''.join(data)
>+
>+


SSService is not a good name imho

>+class SSService(Service):
>+    """Custom Service class to assist DRY in the SyncStorage project.
>+
>+    This Service subclass provides useful defaults for SyncStorage service
>+    endpoints, such as configuring authentication and path prefixes.
>+    """
...

Any reason to have the 'c' variable ? if it's just to be under 80 I guess you could do:

def _ctrl(request):
    return request.registry["syncstorage.controller"]

then:

@storage.delete()
def delete_storage(request):
  return _ctrl(request).delete_storage(request)




>+
>+@storage.delete()
>+def delete_storage(request):
>+    c = request.registry["syncstorage.controller"]
>+    return c.delete_storage(request)
>+
>+
>+@collection.get(accept="application/json", renderer="simplejson")
>+@collection.get(accept="application/newlines", renderer="newlines")
>+@collection.get(accept="application/whoisi", renderer="whoisi")
>+def get_collection(request):
>+    # Force default to application/json.
>+    c = request.registry["syncstorage.controller"]
>+    return c.get_collection(request, **request.GET)
>+
>+
>+@collection.post()
>+def post_collection(request):
>+    c = request.registry["syncstorage.controller"]
>+    return c.set_collection(request)
>+
>+
>+@collection.delete()
>+def delete_collection(request):
>+    c = request.registry["syncstorage.controller"]
>+    return c.delete_collection(request, **request.GET)
>+
>+
>+@item.get()
>+def get_item(request):
>+    c = request.registry["syncstorage.controller"]
>+    return c.get_item(request)
>+
>+
>+@item.put()
>+def put_item(request):
>+    c = request.registry["syncstorage.controller"]
>+    return c.set_item(request)
>+
>+
>+@item.delete()
>+def delete_item(request):
>+    c = request.registry["syncstorage.controller"]
>+    return c.delete_item(request)
Attachment #597745 - Flags: review?(tarek) → review-
(Reporter)

Comment 3

6 years ago
> >+
> >+def includeme(config):
> >+    # Add config settings to use vepauth and basicauth.
> >+    # This is a temporary hack.
> >+    # It will go away once mozsvc gets built-in support for it, but is here
> >+    # for now so that we can test the new auth flow.
>
> Why can't we push those configuration values in the .ini file already ? 

This is just a personal preference of mine, I feel like it should "do the right thing" in the absence of any configuration.  So this sets up the defaults but any or all of them might be overridden in the .ini file.

It mirrors the current setup for using whoauth in mozsvc and server-core, where the use of basic auth is configured in code as the default, and the config file can override it as required.


> >+# the ini file is grabbed at its production place
> >+# unless force via an environ variable
> >+ini_file = os.path.join('/etc', 'syncserver', 'production.ini')
> >+ini_file = os.path.abspath(os.environ.get('INI_FILE', ini_file))
> 
> I find INI_FILE a bit of a generic name for this. I would prefix it with SYNC_STPRAGE maybe

Agreed. This was cut-and-pasted from demoapp so we should probably change it there as well.


> What was wrong with the signals ? 
>

Hmm, I don't recall, perhaps I got a bit overzealous here and need to add some code back in.  Will investigate.

> This seems complicated. Why aren't you using Pyramid events ? They work like the events I
> created in the micro-framework, but based on a @suscriber decorator

The difficulty is that @subscriber decorators are processed at config.scan() time, at which point the CacheManager object might not have been created.  So it's tricky to attach callbacks from the right CacheManager object to the right request.

Also there doesn't seem to be a "the request has finished" event that would fire at the same time as the event in the old system.

Agreed it's yuck though.  I have another thought on how to do it so I'll revisit and try to come up with something cleaner.

> >+    def __init__(self, info):
> >+        pass
> >+
>
> why an empty constructor ? is this signature *required* ?

Yes, pyramid always calls the renderer factory with a single argument.

> SSService is not a good name imho

Fair enough, I will make something longer and split the resulting lines at 80 chars.

> Any reason to have the 'c' variable ? if it's just to be under 80 I guess you could do:

Yes, this was the only reason.
(Reporter)

Comment 4

6 years ago
> >+
> >+def includeme(config):
> >+    """Include all the SyncServer tweens into the given config."""
> >+    config.add_tween("syncstorage.tweens.check_for_blacklisted_nodes")
> >+    config.add_tween("syncstorage.tweens.set_x_timestamp_header")
> >+    config.add_tween("syncstorage.tweens.set_default_accept_header")
>
> Is there a way to configure tweens from the configuration file ?

Yes, although I think it can get a little tricky.  You must remember to include all the tweens from all the packages you are using, including e.g. the tween from cornice and the one from pyramid_whoauth.

> I am asking because this strikes me as we could have a lib of tweens in mozsvc we could reuse,

+1
(Reporter)

Updated

6 years ago
Duplicate of this bug: 720963
(Reporter)

Comment 7

6 years ago
> What was wrong with the signals?

Hmmm, I have to admit, I don't quite know what you're asking here.  What signals are you referring to?
(Reporter)

Comment 8

6 years ago
Created attachment 599050 [details] [diff] [review]
updated patch migrating server-storage to pyramid

OK, I've re-done the event subscription thing to use the higher-level interfaces from pyramid.events.  I'm not sure I like the global nature of it but it's better than using pyramid.threadlocals.

It still uses request.add_finished_callback because this seems to be the recommended way to do it, pyramid doesn't have a RequestFinished event.

Thoughts?
Attachment #597745 - Attachment is obsolete: true
Attachment #599050 - Flags: review?(tarek)
Attachment #597745 - Flags: review?(telliott)
Comment on attachment 599050 [details] [diff] [review]
updated patch migrating server-storage to pyramid

lgtm
Attachment #599050 - Flags: review?(tarek) → review+
(Reporter)

Comment 10

6 years ago
Thanks, committed in https://github.com/mozilla-services/server-syncstorage/commit/90529a7b3cdeacb9c49a47a47ce30d987e0f1c48
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.