Skip to content

Conversation

@bigmstone
Copy link
Contributor

@bigmstone bigmstone commented Jun 29, 2018

(Added / updated by @Kami)

This pull request works on various changes we have introduced over the last couple of releases to introduce fully automatic and dynamic runner loading.

Runners are now dynamically loaded using stevedore on each service startup (during bootstrap / init phase).

This means that st2ctl reload --register-runners command is not needed anymore (same as in the past, but now the runners are also loaded dynamically).

To install a custom runner, user simply needs to create Python package for their runner, install it into StackStorm virtual environment and restart StackStorm services (notablly, action runner).

In addition to that, I also removed support for old deprecated runner names such as run-local, etc. Those names were deprecated over 2 years ago so it's time to finally get rid of them.

TODO

  • Testing cleanup
  • Add new test coverage
  • Update generate requirements to include runners and corresponding st2-packages changes
  • Upgrade notes entries

* Moving get_runner call in container to refer to name instead of module_name
* Using stevedore to load runner in util loader
* Removing tests for removed runner loader
* Removing runner loader based on import
@bigmstone
Copy link
Contributor Author

@Kami take a look at this and let me know if you're good w/ the direction. As you'll see from my error message I plan on making a small st2ctl util to install runners which should allow us to deprecate current runner load methods (packaged, but not installed) as well as <= 2.6 loader methods by just giving a friendly message to run that command. (Also consider 2.9 upgrade command in packaging. [/cc @armab for inspiration]). This, as-is, functions if you install the runner manually to your dev environment's env. I've only tested w/ local runner at the moment.

@bigmstone bigmstone requested a review from Kami June 29, 2018 22:41
@arm4b
Copy link
Member

arm4b commented Jun 30, 2018

I plan on making a small st2ctl util to install runners which should allow us to deprecate current runner load methods (packaged, but not installed) as well as <= 2.6 loader methods by just giving a friendly message to run that command. (Also consider 2.9 upgrade command in packaging.)

Can you describe a little bit in more detail re st2ctl util and its consequences on both packaging and users?

What steps will be required/modified to get the fully working stackstorm system? Will we need an additional st2ctl reinstall-runners step apart of st2ctl reload?
Does this reinstall functionality required only to pick-up optional runners or needed for all core runners?

During the K8s work we experience increasing pain points with any migration, register, reload or init scripts. Any package postinst hooks are flaky by nature, see StackStorm/st2-packages#453 the consequences for st2ctl reload --register-runners which was added in packaging "as a temporary hack" and is still there, spamming errors on every st2 install.
In containerized env, package upgrade/postinst doesn't make sense anymore because we don't do apt/yum upgrade there, but re-package Docker image instead to deliver new version, - it's bundled artifact.

From both systems/installation and user's perspective ideally to have st2 system in a "minimally" working state without any additional register steps.
Thus I'd tend to push in future to register more content as possible by default & on fly on st2 init time, especially if it's core functionality and required for system to run properly.

@Kami As an example, from what I remember some on-fly content reload happens with st2 sensors or it's different?


Let's discuss.

@bigmstone
Copy link
Contributor Author

bigmstone commented Jun 30, 2018

@armab Good comments. Let me clarify a bit.

The reinstall-runners is only for people upgrading from <= 2.8 to >= 2.9. Starting with 2.9 all runners will be packaged with the deb/rpm packages and there's no need to run anything additional. In fact I'm eying a look at removing runners from the DB in general in this PR (I haven't yet because there would be a lot of implications so for now it will stay while I clean the rest up.)

The biggest thing about upgrades I need to check is:

  • What happens to st2 virtualenv when it's upgrade (aka, will new runner wheels be there [I assume so, but I need to find out for sure])
  • If runner wheels are not there what's the best way to get them there. For that I see two options
    1. Post-install script to install-runners (which will basically just install runner modules into st2 venv
    2. st2ctl script that will do the same thing post-install would.

I think once the runners are all loaded via stevedore we can just get rid of them from the contrib folder in general (both in the st2 repo and in the installation contrib). This is why I think we can also get rid of them from the DB (Again there are some implications that might not make it into round 1 of this).

Either way, for HA story, the packages you guys would be installing from will have the runner modules baked into the env. So I do not, at this time, foresee an issue there. The install-runner command will only be added if upgrade process does not include new runner wheels. If it does then we should be okay.

One of my bigger concerns w/ removing runners from the DB (and how this is currently implemented) is I don't know what %age of our packs still have inconsistent naming references to runners. (Note to self: Look through exchange and make those consistent.) So for the foreseeable future we have to provide multiple "references" to a single runner. Or at least we need to deprecate it over the course of some version. Which, perhaps we start that in 2.9?

@Kami
Copy link
Member

Kami commented Jul 9, 2018

Overall, I'm fine with this approach, but I wouldn't touch DB stuff right now since it has many consequences (and some would even argue it's a bad idea from consistency perspective since we also have DB objects for other resources, but that's a long debate :D).

I'm also a bit confused like @armab about the st2ctl command - if I understand things correctly, there should be no need for that, even for users who are upgrading from a previous release.

Runners will be built as Python Wheels and packaged / installed into main StackStorm virtual environment. All the pack virtual environments should inherit dependencies from parent virtualenv (and we also do some PYTHONPATH manipulating) so at least in theory, it should work out of the box.

And I agree with @armab and think this will be an improvement since we can get rid of --register-runners flag / step (well, we will still need to leave flag there for backward compatibility, but it can simply be a no-op).

@Kami
Copy link
Member

Kami commented Jul 9, 2018

Btw, what's our target release for this functionality? v2.9.0?

@Kami Kami added this to the 2.9.0 milestone Jul 9, 2018
@Kami Kami modified the milestones: 2.9.0, 3.0.0 Sep 25, 2018
@Kami
Copy link
Member

Kami commented Sep 25, 2018

What's the plan for this?

I would like to get at least first iteration in v3.0.0. I believe if we finish this PR and make corresponding st2-packages changes to build wheel for each runner and install it in StackStorm virtualenv we don't need any migration scripts.

In addition to that, I would also avoid touching database for now (this would have implications on runner related API endpoint and many other places).

@Kami
Copy link
Member

Kami commented Sep 25, 2018

I went over the code and changes and I believe those are the major changes we need to make to get this to work:

  1. Update runner registrar to discover available runners using stevedore introspection instead of iterating over directories on disk. See tools/enumerate-runners.py for example. I believe all the major changes are already in place so the change itself should be simple.
  2. Update st2-packages to build wheels for each runner package and inject and install it into generated StackStorm virtual environment
  3. Update installer scripts, etc. to remove copy step (cp -R) for runner directories
  4. Update installer and other (test) scripts to install runners into used virtual environment (similar thing as we do for Orquesta)
  5. Document that "behavior" change in upgrade notes

@Kami
Copy link
Member

Kami commented Sep 25, 2018

I will try to get this PR finished - it's mostly there because most of the code was already in place and this PR is just hooking everything up.

I have some issues with existing tests due to how they mock some methods.

I will try to sort this out and then I will look into also making --register-runners flag obsolete.

Initial version of "dynamic runners" feature when this flag was added was a bit rushed. I will make registration implicit. It will happen automatically on each service startup during bootstrap phase (that's how things worked before implementing initial version of dynamic loading feature).

For backward compatibility reasons, I will still leave that flag in place.

during each service setup phase.

This makes it consistent with the approach used before dynamic runner
loading and also makes --register-runners st2-register-content script
flag obsolete.

Flag is still left in place for backward compatibility reasons.
@Kami
Copy link
Member

Kami commented Oct 1, 2018

@bigmstone This PR is now finally finished and ready for a review.

I also needed to update our runner callback and query module loading code so it uses the same dynamic stevedore loading approach.

Whole change took a lot of work - mostly because of mocks and fixtures in our tests (some tests mocked too much, a lot of fixtures were out of date / invalid, etc.).

@Kami Kami merged commit 7a26ace into master Oct 17, 2018
@Kami Kami deleted the stevedore-runners branch October 17, 2018 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants