Move from Mongo/caching to datastore. #44

kadams54 · 2016-12-05T20:54:41Z

Fixes #16

The datastore library (https://github.com/datastore/datastore) lets
Switchboard be much more flexible about how the backend is arranged. Now
Switchboard can support file-based storage for easy local development
and much more complex, cached, sharded, multiple-layered datastores for
production environments.

codecov-io · 2016-12-05T20:59:02Z

Current coverage is 78.69% (diff: 80.36%)

Merging #44 into master will decrease coverage by 6.20%

@@             master        #44   diff @@
==========================================
  Files            22         22          
  Lines          1980       1436   -544   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1681       1130   -551   
- Misses          299        306     +7   
  Partials          0          0

Powered by Codecov. Last update 27e8efa...12401f8

kadams54 · 2016-12-05T21:06:35Z

This PR is marked as a Work In Progress as the docs haven't been updated and there's no migration path from 1.3.7. This PR is not yet ready to land.

kadams54 · 2016-12-06T20:36:43Z

QA

The first step is to make sure the example app works properly in its default configuration; that is, running against a file-based datastore. Note that this is the manual version of what happens when you run make functional-test, which is part of the (currently passing) CI build, so this part is pretty solid.

Activate your virtualenv for Switchboard. If you don't have one, you'll need to run make install to get the requirements installed.
Run make example to startup the example app.
Open your browser to http://127.0.0.1:8080/ - you should see that the example switch is not active.
Open another tab to http://127.0.0.1:8080/_switchboard/ - you should see the admin screen listing an example switch.
Make sure you can add, edit, and delete switches.
Verify that you can add and remove conditions on a switch.
Verify that you can change the status on a switch.
Verify that the inactive status makes the switch inactive (in the first tab).
Verify that the "active for all" status makes the switch active (in the first tab).

Assuming you still have your virtualenv active...

You'll need to have redis installed and fired up locally.
Run pip install datastore.redis
Make this change at the top of example/server.py:

# Setup a file-based datastore with pickle serialization.
# import datastore.filesystem
# import os
# base_path = os.path.dirname(os.path.realpath(__file__))
# ds_file = os.path.join(base_path, '.switches')
# ds_child = datastore.filesystem.FileSystemDatastore(ds_file)
# ds = datastore.serialize.shim(ds_child, pickle)

# Redis-based datastore with pickle serialization.
import redis
import datastore.redis
r = redis.Redis()
ds = datastore.redis.RedisDatastore(r, serializer=pickle)

Once those steps are finished, run through steps 2-9 above to re-test Switchboard against a Redis backend.

frankban

This looks really nice Kyle, I'll try to QA later.

frankban · 2016-12-07T14:14:02Z

example/server.py


+# Setup a file-based datastore with pickle serialization.


What's the reason why these imports cannot be done at the beginning of the file? In the usual 3 import groups?

I originally had these imports up in their usual place, but when I started playing around with connecting to different types of datastores (Mongo, Redis, etc.) I found it handy to have the datastore-specific imports by the code that setup the datastore. Then I could snag the imports when I commented/uncommented a block.

frankban · 2016-12-07T14:14:26Z

example/server.py

+# Setup a file-based datastore with pickle serialization.
+import datastore.filesystem
+import os
+base_path = os.path.dirname(os.path.realpath(__file__))


Please leave a couple of new lines between the imports and the code.

flake8 doesn't actually complain about this... I wonder if it's because I'm assigning variables below the imports and not defining a function or class. I know flake8 complains in those cases. Regardless, if possible, I'd like to keep the imports with the code for easy experimentation with different datastores.

frankban · 2016-12-07T14:16:23Z

example/tests.py

@@ -44,12 +47,16 @@ def assert_switch_inactive(browser, url=url):
 'Switch is not inactive')


+def drop_datastore():


It was Switch.c.drop() before, but I guess this is correct. Maybe add a docstring? [shrug]

Yeah, it was Switch.c.drop() before because Pymongo's Collection class supports a drop command. Datastore (Switch.ds) doesn't support a drop, so to make things easier I created a classmethod Model.drop(). If you look throughout the diff, you'll see all instances of Switch.c.drop() and Model.c.drop() changed to Switch.drop() and Model.drop().

frankban · 2016-12-07T14:17:25Z

setup.py

@@ -19,7 +19,7 @@
 packages=find_packages(exclude=['ez_setup']),
 include_package_data=True,
 install_requires=[
- 'pymongo >= 2.3, < 3',
+ 'datastore',


I'd add a >= constraint here, just to document the minimum supported version of datastore.

frankban · 2016-12-07T14:19:38Z

example/tests.py

@@ -182,22 +189,6 @@ def test_add_edit_delete_switch(self):
 wait_time=10)
 assert_true(is_deleted, 'Switch was not deleted.')

- def test_switch_history(self):


So I guess, as part of this simplification, you are removing a switch history feature?
Was that used/working before?

Yeah, I removed it because it wasn't going to work as architected with Datastore; however, it is used, so I'm planning on re-adding support in a subsequent PR. It will likely be a VersionedDatastore that can be optionally enabled.

frankban · 2016-12-07T14:36:18Z

switchboard/base.py


-NoValue = object()
+ mydict = ModelDict(Model, value='foo')
+ mydict['test']


Where does "test" come from? It's not clear from your example, as it states that a model that has a attribute named foo where the value is "bar".

frankban · 2016-12-07T14:40:06Z

switchboard/base.py

+
+ def __setitem__(self, key, value):
+ if isinstance(value, self.model):
+ nested_value = getattr(value, self.value)


Can't this raise an AttributeError?

frankban · 2016-12-07T14:40:37Z

switchboard/base.py

+ instance, created = self.model.get_or_create(key, defaults=defaults)
+
+ # Ensure we're updating the value in the datastore if it changes.
+ if getattr(instance, self.value) != nested_value:


Again, why we can be sure that a filed named self.value is in instance? Please add comments.

frankban · 2016-12-07T14:45:40Z

switchboard/models.py

 def save(self):
- if hasattr(self, '_id'):
- previous = self.get(_id=self._id)
+ if hasattr(self, 'key'):


Or alternatively:

try: key = self.key except AttributeError: key = self.key = str(uuid.uuid4()) previous = None else: previus = self.get(key)

frankban · 2016-12-07T14:47:39Z

switchboard/models.py

+ '''
+ try:
+ import datastore.redis
+ return isinstance(cls.ds, datastore.redis.RedisDatastore)


Please move the return at the end, so that only the code that we want to protect for that exception is in the try/except block.

frankban · 2016-12-14T08:46:43Z

I encountered the following error while running "make install".

Failed building wheel for smhasher
  Running setup.py clean for smhasher
Successfully built coverage splinter datastore blinker Mako
Failed to build smhasher
Installing collected packages: nose, six, funcsigs, pbr, mock, coverage, bottle, paste, selenium, splinter, smhasher, datastore, blinker, WebOb, MarkupSafe, Mako, switchboard
  Running setup.py install for smhasher ... error
    Complete output from command /home/frankban/code/garbage/switchboardvenv/bin/python -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-C1rb6J/smhasher/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile
(code, __file__, 'exec'))" install --record /tmp/pip-4XhBoA-record/install-record.txt --single-version-externally-managed --compile --install-headers /home/frankban/code/garbage/switchboardvenv/include/site/python2.7/smhasher:
    running install
    running build
    running build_ext
    building 'smhasher' extension
    creating build
    creating build/temp.linux-x86_64-2.7
    creating build/temp.linux-x86_64-2.7/smhasher
    x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DMODULE_VERSION="0.136.2" -Ismhasher -I/usr/include/python2.7 -c smhash
er.cpp -o build/temp.linux-x86_64-2.7/smhasher.o
    cc1plus: warning: command line option ‘-Wstrict-prototypes’ is valid for C/ObjC but not for C++
    x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DMODULE_VERSION="0.136.2" -Ismhasher -I/usr/include/python2.7 -c smhash
er/MurmurHash3.cpp -o build/temp.linux-x86_64-2.7/smhasher/MurmurHash3.o
    cc1plus: warning: command line option ‘-Wstrict-prototypes’ is valid for C/ObjC but not for C++
    smhasher/MurmurHash3.cpp:81:23: warning: always_inline function might not be inlinable [-Wattributes]
     FORCE_INLINE uint64_t fmix ( uint64_t k )
                           ^
    smhasher/MurmurHash3.cpp:68:23: warning: always_inline function might not be inlinable [-Wattributes]
     FORCE_INLINE uint32_t fmix ( uint32_t h )
                           ^
    smhasher/MurmurHash3.cpp:60:23: warning: always_inline function might not be inlinable [-Wattributes]
     FORCE_INLINE uint64_t getblock ( const uint64_t * p, int i )
                           ^
    smhasher/MurmurHash3.cpp:55:23: warning: always_inline function might not be inlinable [-Wattributes]
     FORCE_INLINE uint32_t getblock ( const uint32_t * p, int i )
                           ^
    smhasher/MurmurHash3.cpp: In function ‘void MurmurHash3_x86_32(const void*, int, uint32_t, void*)’:
    smhasher/MurmurHash3.cpp:55:23: error: inlining failed in call to always_inline ‘uint32_t getblock(const uint32_t*, int)’: function body can be overwritten at link time
    smhasher/MurmurHash3.cpp:112:36: error: called from here
         uint32_t k1 = getblock(blocks,i);
                                        ^
    smhasher/MurmurHash3.cpp:68:23: error: inlining failed in call to always_inline ‘uint32_t fmix(uint32_t)’: function body can be overwritten at link time
     FORCE_INLINE uint32_t fmix ( uint32_t h )
                           ^
    smhasher/MurmurHash3.cpp:143:16: error: called from here
       h1 = fmix(h1);
                    ^
    smhasher/MurmurHash3.cpp:55:23: error: inlining failed in call to always_inline ‘uint32_t getblock(const uint32_t*, int)’: function body can be overwritten at link time
     FORCE_INLINE uint32_t getblock ( const uint32_t * p, int i )
                           ^
    smhasher/MurmurHash3.cpp:112:36: error: called from here
         uint32_t k1 = getblock(blocks,i);
                                        ^
    smhasher/MurmurHash3.cpp:68:23: error: inlining failed in call to always_inline ‘uint32_t fmix(uint32_t)’: function body can be overwritten at link time
     FORCE_INLINE uint32_t fmix ( uint32_t h )
                           ^
    smhasher/MurmurHash3.cpp:143:16: error: called from here
       h1 = fmix(h1);
                    ^
    error: command 'x86_64-linux-gnu-gcc' failed with exit status 1

brondsem

I've taken a look through the code and made a few inline comments. Overall I think this is a good set of changes, and the datastore system should have enough flexibility for caching, etc (nice to get rid of all the internal caching logic!) I do have a few requests:

can we have a ticket for adding versioning (or at least a simple audit log/list of changes)? It would be good to reference back to the commit that removed the versioning since some tests and UI support at least can be restored at that point.
what about import/export of switches to facilitate migrating between datastores? (Also would be handy to allow copying switches between environments). Export functionality would probably be best in a 1.3.x branch release too so that we can migrate data forward. A separate ticket is fine for that, but will be a blocker for SourceForge to update. Or I guess a datastore implementation that saves mongo data in the same format as before, but offhand I'd guess that's not trivial.

brondsem · 2016-12-14T20:16:59Z

switchboard/base.py


 def __repr__(self): # pragma: nocover
 return "<%s>" % (self.__class__.__name__)

 def iteritems(self): # pragma: nocover
- self._populate()
- return self._cache.iteritems()
+ return zip(self.iterkeys(), self.itervalues())


This isn't very efficient, since iterkeys() and itervalues() will each end up calling self._model.all(). Pratically though, I'm not sure if this method is really used anywhere that would matter.

brondsem · 2016-12-14T20:22:25Z

switchboard/models.py

 return result

 @classmethod
 def all(cls):
- return [cls(**s) for s in cls.c.find()]
+ if cls._is_redis_datastore():


Checking specifically for redis here isn't ideal. It makes me think other datastores could suffer from the same limitations (lack of querying). Can we feature-detect query support rather than redis specifically?

Good call. I'll switch to a try/except on NotImplemented or something similar.

frankban · 2016-12-15T14:54:38Z

QA:
This is mostly ok, I tried the "active for everyone" condition and it worked,
then I tried with conditions, like querystrings, hosts etc.
Everything works with specific conditions until you click "exclude". My understanding is that by clicking "exclude" we are reverting the condition, for instance "the flag is active if the host is not bad-host". However, the switch is never active in those cases. I wonder if this is a pre-existing issue, in which case I'd suggest to land this and work on a follow up specific to that problem.

Redis:
Is it possible to QA this having a redis server running on a LXD container?
Anyway, I was not able to QA redis as pip install datastore.redis tries to install smhasher==0.136.2 (from datastore>=0.3.3->datastore.redis) and therefore it gave me the same error encountered in the previous QA attempt.

brondsem · 2016-12-15T15:18:44Z

re: exclude conditions - in my experience you can't have just an exclude condition. You need to have at least one condition match and none of the exclude conditions match. (We often have a ".*" positive match to go with the NOT "whatever"). So I agree it's pre-existing and a separate issue for making that work more intuitively would be nice.

kadams54 · 2016-12-15T16:56:17Z

@brondsem and @frankban: I've created issues #45, #46, and #47 to cover various problems discussed in this PR. At the very least, I'd see #45 and #46 being addressed quickly, before a 2.0 release. That said, I'd like to do that work separate from this PR, in part because this is a pretty significant hunk of work.

brondsem · 2016-12-15T18:59:29Z

Things look good here to me

Fixes switchboardpy#16 The datastore library (https://github.com/datastore/datastore) lets Switchboard be much more flexible about how the backend is arranged. Now Switchboard can support file-based storage for easy local development and much more complex, cached, sharded, multiple-layered datastores for production environments.

kadams54 · 2016-12-16T14:39:21Z

Thanks all! Going to ship this and move on to smoothing the rough edges and prepping for a 2.0 release.

kadams54 force-pushed the datastore branch from 41fffc8 to b25a2ee Compare December 5, 2016 20:57

kadams54 added the do not merge label Dec 6, 2016

kadams54 force-pushed the datastore branch from 1dff234 to ef071df Compare December 6, 2016 20:48

frankban approved these changes Dec 7, 2016

View reviewed changes

kadams54 removed the do not merge label Dec 13, 2016

kadams54 changed the title ~~WIP - Move from Mongo/caching to datastore.~~ Move from Mongo/caching to datastore. Dec 13, 2016

brondsem requested changes Dec 14, 2016

View reviewed changes

brondsem approved these changes Dec 15, 2016

View reviewed changes

kadams54 added 2 commits December 15, 2016 15:35

Update to latest Selenium.

12401f8

kadams54 force-pushed the datastore branch from 412b99e to 12401f8 Compare December 15, 2016 20:35

kadams54 merged commit 0ef5610 into switchboardpy:master Dec 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from Mongo/caching to datastore. #44

Move from Mongo/caching to datastore. #44

kadams54 commented Dec 5, 2016

codecov-io commented Dec 5, 2016 •

edited

Loading

kadams54 commented Dec 5, 2016

kadams54 commented Dec 6, 2016

frankban left a comment

frankban Dec 7, 2016

kadams54 Dec 7, 2016

frankban Dec 7, 2016

kadams54 Dec 7, 2016

frankban Dec 7, 2016

kadams54 Dec 7, 2016

frankban Dec 7, 2016

frankban Dec 7, 2016

kadams54 Dec 7, 2016

frankban Dec 7, 2016

frankban Dec 7, 2016

frankban Dec 7, 2016

frankban Dec 7, 2016

kadams54 Dec 13, 2016

frankban Dec 7, 2016

frankban commented Dec 14, 2016

brondsem left a comment

brondsem Dec 14, 2016

brondsem Dec 14, 2016

kadams54 Dec 15, 2016

frankban commented Dec 15, 2016

brondsem commented Dec 15, 2016

kadams54 commented Dec 15, 2016

brondsem commented Dec 15, 2016

kadams54 commented Dec 16, 2016

		@@ -44,12 +47,16 @@ def assert_switch_inactive(browser, url=url):
		'Switch is not inactive')


		def drop_datastore():

Move from Mongo/caching to datastore. #44

Move from Mongo/caching to datastore. #44

Conversation

kadams54 commented Dec 5, 2016

codecov-io commented Dec 5, 2016 • edited Loading

Current coverage is 78.69% (diff: 80.36%)

kadams54 commented Dec 5, 2016

kadams54 commented Dec 6, 2016

QA

frankban left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankban commented Dec 14, 2016

brondsem left a 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

frankban commented Dec 15, 2016

brondsem commented Dec 15, 2016

kadams54 commented Dec 15, 2016

brondsem commented Dec 15, 2016

kadams54 commented Dec 16, 2016

codecov-io commented Dec 5, 2016 •

edited

Loading