[merged] Store handler manager

[mbarnes]

This ties in to CPD-126 as an extra layer between CherryPyStorePlugin and various StoreHandler instances. The idea is StoreHandlerManager can be cloned with an identical configuration and handed off to child processes so they can read and write to the appropriate stores while avoiding the forked CherryPy bus.

A couple small deviations from CPD-126:

1. The register_store_handler() method takes a type object and a configuration object instead of a StoreHandler instance. The type object should be derived from the StoreHandlerBase class. StoreHandlerManager will lazily create StoreHandler instances as needed, and these instances are NOT duplicated in cloned StoreHandlerManagers.
2. Consequently, StoreHandler instances can go ahead and connect to their stores immediately. So the _get_connection() method is no longer needed.

Note the API still takes etcd key paths instead of model instances. This is temporary so we can implement and test CPD-126 incrementally.

@ashcrow: Code's ready for review if you have time. So far so good with BDD and manual tests. Unit tests are always a pain; I'll finish them Friday.

[ashcrow]

Reading through this change so far I am questioning the need for CherryPyStorePlugin changes. StoreHandlerManager can meet the same need through a slightly different pattern on the bus as shown in this PR. Thoughts?

I'm not against updating cpd-126 a bit if needed.

[mbarnes]

Reading through this change so far I am questioning the need for CherryPyStorePlugin changes. StoreHandlerManager can meet the same need through a slightly different pattern on the bus as shown in this PR. Thoughts?

I think I kind of addressed this in my inline comment.

[ashcrow]

But that got me thinking: with these changes the CherryPy StorePlugin is really just a portal to a global StoreHandlerManager instance -- the plugin methods are all trivial passthroughs. Maybe the plugin API should just have a get-store-manager method, or maybe we don't really need a plugin at all if there's a better Python idiom for global resources in an app.

I think changing the store plugin to have get_store_manager makes sense. The other option would be to go with a globals file, similar to the jobs module. This is the common pattern for globals, though I've had mixed results with it in Python 2.

I don't know if I actually answered your question. This is me overthinking things now. 😄

As engineers, sometimes I think we are paid to overthink 😆

[mbarnes]

I think changing the store plugin to have get_store_manager makes sense. The other option would be to go with a globals file, similar to the jobs module. This is the common pattern for globals, though I've had mixed results with it in Python 2.

That's a helpful link, thanks! A globals file seems simple enough.

I don't know offhand why Python 2 wouldn't work with that pattern, but if it's gonna be a bugger with that too then a somewhat lame workaround might be a "globals" CherryPy plugin with a bunch of "get" methods, and just request globals over the bus. (Main process only, to be clear.)

[ashcrow]

I actually think the pattern you have implemented here with the get_store_manager will work just fine!

[mbarnes]

Okay cool, I'll add some fixups.

[mbarnes]

@ashcrow Do you mind if I defer the CherryPyStorePlugin rework to a followup PR? I'd rather do this in stages, otherwise I think the fixup commits are going to get really confusing.

[mbarnes]

:arrow_up: I owe you additional test cases for StoreHandlerManager, but the existing tests for CherryPyStorePlugin, investigator and clusterexec exercise it pretty well.

Also I don't want CPD-126 to be blocked on this PR while I'm gone for a week.

[ashcrow]

Follow on for additional work is fine with me.

Let me know what test cases are missing and I'll do them as another follow on if you run out of time.

[mbarnes]

Model type registration tests are missing. Basically call register_store_handler() for a variety of handler types and model types and then make sure _get_handler() picks the right handler for a model. (For this test the handler and model types don't even have to be legit... just trivial class definitions should work.)

Since I'm only registering one handler and a phony model type at the moment, that whole mechanism isn't really getting coverage.

[ashcrow]

OK. I'll take a look at that after reviewing once more.

[ashcrow]

Still looks good to me. I'm going to run a few manual tests before merging. Shouldn't take more than 30 minutes at most.

[ashcrow]

:+1:

I'll take care of the additional tests early next week.

@rh-atomic-bot r+

[rh-atomic-bot]

:pushpin: Commit bd9ba1c has been approved by ashcrow

[rh-atomic-bot]

:hourglass: Testing commit bd9ba1c with merge 5c0b65f...

[rh-atomic-bot]

:sunny: Test successful - travis Approved by: ashcrow Pushing 5c0b65f3a7385daef3366a6257475cae35caf6a3 to master...

[ashcrow]

Trello card for follow on: https://trello.com/c/ZDfyE6sd

[ashcrow]

Tagging https://github.com/projectatomic/commissaire/issues/126