Separate storage mechanism for routes #81

pseudomuto · 2016-09-28T18:07:16Z

The Problem

Currently the code that handles persistence of routes is embedded directly in the proxy code. This makes it difficult to choose a different storage method (e.g. redis, memcached, db, etc.).

This issue becomes apparent when you're running multiple JupyterHub instances behind nginx (or some other proxy). In this scenario, each JH instance will spawn their own proxy and maintain their own route table (in the node proc).

When a request comes in at /user/xyz, depending on the host that nginx routes to, the local proxy will either have the route, or not.

😢

Initially I patched the JH Proxy object to update all routes and lowered the last_activity_timeout param so it would happen often, but this seemed like a total hack and doesn't always work (still a small window of time where the error can occur).

The Solution

Ideally, storage should be configurable/pluggable. I'm thinking something like passing a config option to change the persistence method or something.

In order to keep this PR small and reviewable, I've simply moved the storage from configproxy.js into it's own store.js module. The trie is still used, and all storage is still in memory (i.e. nothing changes).

Next Steps

If this seems reasonable, then I'll add implementations for other providers in a separate PR.

pseudomuto · 2016-09-28T18:14:08Z

CI failure seems to be flaky tests. This one for instance passed on the retry, but failed initially

rgbkrk · 2016-09-28T18:17:27Z

Great, thanks for bringing this up! I agree that we need to have a way to persist routes reliably across many nodes.

minrk · 2016-09-28T19:17:43Z

Thanks! I think it's fine to add this functionality. I'm not sure the best way to go about it. The things I'd be concerned about are:

what's the best way to add providers? I don't want CHP to grow too much, and adding a provider in general shouldn't have to be a PR against this repo, and I don't love adding dependencies that people don't use.
I would prefer to avoid route lookup becoming an async operation, but it would presumably have to be for any external storage, and therefore also become an immediately-evaluated async API for the in-memory case, as well.

I don't know how to achieve 1 best, given how npm bundles dependencies. I'd also be worried about growing optional dependencies, but at least the redis and memjs clients aren't problematic (other memcache clients appear to be).

pseudomuto · 2016-09-28T19:33:33Z

@minrk I agree with all of that!

I'm just working on making the storage async as we speak. As for adding deps/providers in this repo, I'm not entirely sure what the best move would be just yet. One of those "I'll burn that bridge when I get there" kinda things 😄

Maybe peer dependencies could be used somehow?

minrk · 2016-09-28T19:41:27Z

I don't think the tests have been flaky recently, so that seems like there might be something amiss in the changes here, especially since every run is failing in the same way, and a push from master just now is all green. This PR does pass all tests when I run on my laptop, though, which makes debugging harder.

Re: peerDependencies, maybe! I'm far from an expert on npm packaging, and mostly get frustrated the more I try to solve nontrivial problems with it. My first impression is that peerDeps would solve it well where configproxy is used as a library, but may not work well for the npm install -g-style application used most often. I would love for someone else to figure out the best way to deal with that. Let us know what you figure out!

pseudomuto · 2016-09-29T18:27:40Z

It's 🍏 !!!

I've updated the code to now use an async API for storage. Not gonna lie, it was a little rough. This is much bigger than I originally intended, so Iet me apologize in advance 😄

I don't think the tests have been flaky recently, so that seems like there might be something amiss in the changes here

This was indeed my changes. The first was actually a lint error, and the 0.x versions didn't support Object.assign.

minrk · 2016-09-30T09:07:54Z

You're right, this is really huge! The async changes are unfortunate, but I guess there's no way around it.

It apparently also contains a bunch of aesthetic changes? I wish that didn't happen, as the PR is big enough without those, especially since in most cases I prefer the before to the after. Was there a reason for those? Or did you use an auto-formatter?

minrk · 2016-09-30T09:10:00Z

lib/configproxy.js

+
+        if (inactive_since) {
+            var keys = Object.keys(routes).filter(function (key) {
+                return routes[key].last_activity < inactive_since;


The previous implementation was more efficient because it iterated over the routes only once, rather than twice.

minrk · 2016-09-30T09:11:27Z

lib/configproxy.js

+
+    this._routes.getTarget(base_path + decodeURIComponent(req.url), function (route) {
+        timer.stop();
+        var result = route ? { prefix: route.prefix, target: route.data.target } : null;


It would be nice to leave the less-dense format above.

minrk · 2016-09-30T09:27:53Z

test/api_spec.js

        ];

-        r.get(api_url + "?inactive_since=endoftheuniverse", function (error, res, body) {


This check was removed. Can it come back?

I just moved it to it's own test (above)

Sorry I missed that, thanks!

minrk · 2016-09-30T09:28:38Z

test/api_spec.js

-            },
+            { name: 'long ago', since: long_ago, expected: {} },
+            { name: 'an hour ago', since: hour_ago, expected: { '/yesterday': true } },
+            { name: 'the future', since: hour_from_now, expected: { '/yesterday': true, '/today': true } }


FWIW, I would prefer this kind of style change to not happen (before is better than after, for me).

minrk · 2016-09-30T09:32:37Z

test/api_spec.js

+        var now           = new Date();
+        var yesterday     = new Date(now.getTime() - (24 * 3.6e6));
+        var long_ago      = new Date(1);
+        var hour_ago      = new Date(now.getTime() - 3.6e6);


I haven't done aligned assignments generally, and would prefer them not to be applied to existing code without any other changes. Not a big deal, just something for the future.

pseudomuto · 2016-09-30T13:12:48Z

Updated to remove all of the style-only changes (align assignments, etc.). I've also rebase on the latest master with @rgbkrk's changes

rgbkrk · 2016-09-30T14:18:43Z

lib/store.js

@@ -62,7 +62,12 @@ function MemoryStore () {
    },
    update: {
      value: function (path, data, cb) {
-        Object.assign(routes[path], data);


We could probably go back to this to simplify the logic in a later PR.

willingc · 2016-09-30T15:44:12Z

Thank you @pseudomuto 😎

pseudomuto force-pushed the separate_route_storage branch 3 times, most recently from e39e45b to 9f03146 Compare September 29, 2016 18:16

minrk reviewed Sep 30, 2016

View reviewed changes

pseudomuto added 7 commits September 30, 2016 08:51

Add MemoryStore for in-memory route table

9f9cdc3

Use new store in configproxy.js

c105494

Let the store handle updates

d2490b0

Move trie use into store

3054ccf

Add BaseStore object

a2a0b47

Make it all async

faf3dfc

Object.assign not available in 0.1x releases

7f7c2b1

pseudomuto force-pushed the separate_route_storage branch from 9f03146 to 4028d31 Compare September 30, 2016 13:02

Revert style-only changes

58446c1

pseudomuto force-pushed the separate_route_storage branch from 4028d31 to 58446c1 Compare September 30, 2016 13:09

minrk merged commit 3ec36da into jupyterhub:master Sep 30, 2016

pseudomuto deleted the separate_route_storage branch September 30, 2016 14:13

rgbkrk reviewed Sep 30, 2016

View reviewed changes

pseudomuto mentioned this pull request Sep 30, 2016

[WIP] Add the Ability to use Redis for Storage #85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate storage mechanism for routes #81

Separate storage mechanism for routes #81

pseudomuto commented Sep 28, 2016

pseudomuto commented Sep 28, 2016

rgbkrk commented Sep 28, 2016

minrk commented Sep 28, 2016

pseudomuto commented Sep 28, 2016

minrk commented Sep 28, 2016

pseudomuto commented Sep 29, 2016

minrk commented Sep 30, 2016

minrk Sep 30, 2016

minrk Sep 30, 2016

minrk Sep 30, 2016

pseudomuto Sep 30, 2016

minrk Sep 30, 2016

minrk Sep 30, 2016

minrk Sep 30, 2016

pseudomuto commented Sep 30, 2016

rgbkrk Sep 30, 2016

willingc commented Sep 30, 2016

		];

		r.get(api_url + "?inactive_since=endoftheuniverse", function (error, res, body) {

Separate storage mechanism for routes #81

Separate storage mechanism for routes #81

Conversation

pseudomuto commented Sep 28, 2016

The Problem

The Solution

Next Steps

pseudomuto commented Sep 28, 2016

rgbkrk commented Sep 28, 2016

minrk commented Sep 28, 2016

pseudomuto commented Sep 28, 2016

minrk commented Sep 28, 2016

pseudomuto commented Sep 29, 2016

minrk commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pseudomuto commented Sep 30, 2016

Choose a reason for hiding this comment

willingc commented Sep 30, 2016