Datastore access for python actions

[vcabbage]

Extracts the datastore access logic from SensorWrapper into DatastoreService. An instance of DatastoreService is provided to python action runners by adding it to the class instance in PythonActionWrapper._get_action_instance. Would have preferred passing it to the Action initializer, but doing so requires changing the initializer signature which breaks actions that have overridden the initializer.

The DatastoreService is added to SensorService. The public datastore access methods are wrapped on SensorService to prevent changing the public API.

In #2195 there was some concern about how this functionality would be used. One of the use cases that I see as legitimate is caching data that may take considerable time to pull from a source system. Using it to pass data between actions and/or sensors is probably not an ideal use case.

• Is st2common/services/datastore.py an appropriate place for the DatastoreService?
• Resolved test issued in st2reactor/tests/unit/test_sensor_wrapper.py, should the datastore tests be moved to their own test file?

Side note: The API between sensors and python actions is inconsistent (ie, on sensors the configuration is accessed as self._config, whereas it's self.config on Actions). I added the DatastoreService as self.datastore to maintain consistency within the Action class. It would be nice if sensors and actions were more inline. Perhaps picking one option and deprecating the other?

[lakshmi-kannan]

Not sure why you went with stdlib logging instead of st2common logging that's already used everywhere else.

You just need to import like so "from st2common import log as logging" in DatastoreService. Also, passing in a logger seems not needed at that point. Am I missing something?

[manasdk]

This is more to do with usage in _set_up_logger as logging from st2common is still used in this module.

[vcabbage]

I forgot I had done that. It's from a quick attempt at getting this to log in the same file as the action, _set_up_logger is a copy and paste from the Action class. I didn't intend to submit with that code. (This is also the answer to @manasdk's question about _set_up_logger.)

Do you have a suggestion on how to accomplish the same goal? Assuming logging into the same files is desired.

As to why it's being passed in, in SensorService it was using the logger from the instance of SensorWrapper that gets passed in. I wanted to avoid changing SensorService's behavior.

[lakshmi-kannan]

ie, on sensors the configuration is accessed as self._config, whereas it's self.config on Actions

Yep. We should be consistent wherever possible. To deprecate, we should add self.config in sensors and then deprecate ._config (may not happen soon). Please feel free to open an issue for this.

[lakshmi-kannan]

Is st2common/services/datastore.py an appropriate place for the DatastoreService?

Yes. I am +1 to this.

Resolved test issued in st2reactor/tests/unit/test_sensor_wrapper.py, should the datastore tests be moved to their own test file?

+1 to new file in st2common/tests/unit/test_datastore.key

[lakshmi-kannan]

Please update the CHANGELOG and add documentation in https://github.com/StackStorm/st2docs/blob/master/docs/source/actions.rst. Maybe we should create a new file python_actions and a section about accessing datastore? cc: @Kami, @manasdk

[manasdk]

Setting on the class rather than the instance does not seem right which is assume would be inherited from https://github.com/StackStorm/st2/blob/master/st2actions/st2actions/runners/pythonrunner.py#L63.

Also, no datastore property on the base class either - am I missing something?

[vcabbage]

You're right, I intended to set it on the instance, not the class. I will correct it.

[vcabbage]

@lakshmi-kannan @manasdk I think I've addressed the issues brought up, except for logging.

My thought on logging is that since PythonRunner captures stdout/stderr, keep the _set_up_logger method on PythonActionWrapper and remove it from Action to reduce code duplication. Then PythonActionWrapper can set the logger on the Action and DatastoreService instances. Thoughts?

[manasdk]

@vcabbage

My thought on logging is that since PythonRunner captures stdout/stderr, keep the _set_up_logger method on PythonActionWrapper and remove it from Action to reduce code duplication.

Agreed. I suggest https://github.com/Plexxi/st2/pull/6/files

[vcabbage]

Thanks @manasdk! You're suggestions have been pulled in, the PR is ready for another review.

[manasdk]

Looks good to me. Merging!

I see docs StackStorm/st2docs#23 and will merge that once/if Travis green lights.

Thanks for the PR.

[Kami]

Sweet, and thanks again! :)

[Kami]

@vcabbage @andrew-regan

First I want to say thanks again for this contribution and great work on it.

When this PR was originally active, I was a bit busy with installer and some other work so I didn't have time too dig in too much.

I noticed that some of the changes (notably late assignment of datastore and logger variable on the action instance) don't play along too nicely with our current pack testing story so I decided to make some changes - #2511 (docs at StackStorm/st2docs#67).

In those changes I also decided to unify the interface for sensors and Python runner actions. Now we pass action_service object to Python runner actions and user accesses datastore via action_service object.

This way it's now consistent with sensors and sensor_service.

Those changes are not backward compatible with your original changes, but hopefully it won't be too much of a hassle to update your existing actions (let us know if you need assistance).

I also added some tests (previously we didn't have any such tests in st2 repo and there was a bug in the Makefile in st2contrib repo so we didn't notice build failures) so in the future we will notice and address potentially breaking changes earlier.