RFC: Simple callback system for users


#1

There’s Foreman Hooks plugin which allows users to hook arbitrary scripts on various ActiveRecord and Foreman orchestration actions. The problem with this approach is that this is deeply integrated with the two technologies, users need to understand them and check scripts after major Rails upgrades because behavior of ActiveRecord changes over time.

More importantly, the scripts are being called during ActiveRecord handling when records or their associacions are not yet saved, this can lead to fields not being set. Particularly with Foreman Orchestration which is by design during validation. Users cannot find out if model was created, updated or deleted when they hook to some common callbacks as well.

Current hooks are actually part of transactions which can lead to a rollback, this is a feature which creates more problems when upgrading or dealing with incompatibility. Last but not least, the plugin is currently unmaintained. Anyone is more than welcome to take it and improve.

The proposal:

Provide an alternative and simple callback mechanism based on Rails controller actions rather than model by creating an application generic around filter. For each controller/action the filter publishes an event. No data from our database is sent along the event, subscribers are expected to use our standard API to find relevant records. Key features:

  • Before callbacks send all request parameters along with an event.
  • After callbacks send database record primary ID along with an event (but not more).
  • Callback API available through the application for ad-hoc callback points when necessary.
  • Subscribers are running outside of Rails application on purpose.
  • Core feature with tests, not a plugin.

Now, publishing those events. I was thinking ActionCable, it’s in Rails 5, easy to use, documented and there are 3rd party libraries for subscribing and consuming the content like https://github.com/tobiasfeistmantl/python-actioncable-zwei. The problem is that ActionCable is not supported on Passenger running on Apache.

Alternatively, this could be as simple as publishing into Redis via low-level Ruby client and picking that up using script. We were discussing possibility of adding Redis into the core stack, that could be nice use of it (no ActionCable or websockets involved).

Another way is reusing our statsd telemetry stack and publishing events via UDP datagrams. Looks like statds supports an extension for that, unfortunately Prometheus does not.

Finally, another option is to call HTTP endpoint, that’s more straightforward but this will scale worse. Generally we should avoid making HTTP calls when processing web requests.

Wrap up

This does not look like lot of work, if we agree on the best publishing mechanism I’d like to proceed and create a prototype to see how it works in real use. Then we can decide what to do next. It’s important to know that this does not aim to replace Foreman Hooks, but to complement it. The whole designed learned from my experience with debugging and supporting various problems with foreman_hooks, it aims to be as simple as possible.

I am open to ideas and comments.


#2

I’ve been thinking about changing our deployment to a standalone application server (likely puma) + reverse proxy setup. I hope that’ll allow ActionCable/websockets to work and also simplify our webserver config. It also closer aligns to the containerization effort. The downside is that we’ll need to relearn tuning the setup and provide an easy migration path. It’s not very concrete yet.


#3

I guess we could follow similar pattern as with telemetry, by having different sinks for the events. +1 for trying web-sockets or redis. I’m not sure if using telemetry would be a good fit. Btw. couldn’t we provide the ‘subscriber part’ as well, that would wrap the custom bash scripts the user provides? This would even more decouple the implementation and usage.


#4

I did a little experiment with wisper gem offering pub-sub capabilities, because I wanted a plugin to be aware when a hostgroup was created. I am not sure what you plan for callback API, but I will welcome a solution where I can completely avoid extending models with ActiveRecord callbacks.


#5

So I took a Friday chance to take a look on wisper and played around a bit and while I liked the idea very much, after some time spent in the codebase I am not convinienced that this is the right solution for us.

First, it is pub-sub library which is both sync and async, this makes it utterly complex. I would not mind using term “overengineering” although most of it is probably tax for flexibility and ruby metaprograming.

Second, there are some concerns about performance of this library, particularly for larger RoR applications which we really are (ActiveRecord::Base.ancestors.size is 80). I don’t want to be debugging performance bottlenecks in a code responsible for delivering messages across application or outside of application (this must be blazing fast from the day one).

Finally, I have some concerns about decoupling plugins via pub/sub. In your WIP code, you actually pass activerecord instance through (broadcast(:a_message, self)) which is far from decoupling. Even passing database ID is not improving this, we would need to dramatically rewrite parts of our codebase to be able to provide an API with ability to run plugins separately (in separate processes/containers).

For these reasons, I think wisper is not a good library to go with for our simple callback systems. I believe writing couple of classes our own would do the job and decoupling Foreman Plugins is a topic for different time I guess.

I’ve created a ticket, yet I don’t have any working code as I am killing this idea for today:

https://projects.theforeman.org/issues/25247


#6

Those are valid points, thanks for looking into this.


#7

Just leaving here another rather surprising fact - you can’t use foreman_hooks with Katello, at least ContentView hook “after_create” does not work:

[W|app|25463] Unable to render Test3 (Katello::ContentView) using RABL: Cannot find rabl template 'api/v2/katello/contentviews/show' within registered ...

Obviously, hook plugin assumes there is an existing RABL template for each model which is only the case for Foreman core and some plugins.


#8

I had an idea today to hook the callback mechanism on audit events. The idea was simple, user can nicely see all audit records via UI/API, hooking on that is simple, possibility to add “Resend” button like in Foreman Tasks. But I assumed that audits are after_commit, they are obviously after_create etc. That killed my idea.

However I think that visibility and easy debugging should be feature from the very beginning. For this reason I think it might be good idea to have Foreman Tasks in the stack. So we should either do the initial implementation completely based on Tasks, or find a way to integrate later easily (small core + option to deliver messages via tasks).

Another thing I was thinking about was if we want to hook callbacks via controller/action or hook it via after_commit. The former needs some extra dance around figuring out resource ID which must be passed into the callback while after_commit should be relatively safe since transaction was completed. The latter approach has one advantage which I admit I want to avoid works across entrypoints (UI, API). On the other hand, we should be very explicit and on purpose avoid making the callback mechanism work automatically with all resources.

I also think that distributed Ruby (drb) could be the initial implementation of the transfer mechanism for the first cut at least. We don’t know when/if Redis lands the core.


#9

One quick idea, how about exposing audit events via tasks? Foreman tasks
would extend audited gem with after commit callback. That is still one
endpoint to extend but out of active record transaction. The task would
have the link to audit and audited record. Or we could add after commit
callback to audited gem if that helps.


#10

I like this idea because I keep hearing folks would like to audit more things. If added hooks to it, then they support each other. However, we may have to deal with some things which are very lifecycle specific:

  1. A callback which distinguishes between starting a provision and it ending
  2. A callback which distinguishes between the start of a sync and an end

#11

After_commit callback for audit event still happens during uncommited transaction of a resource. That does not take us anywhere I think.


#12

Why do you think so? Based on
this
it’s out of transaction. There’s after_commit amd after_rollback that we
could use.

Though as Bryan said, audit events may not be granular enough, e.g.
provisioning start vs end. Not sure if we audit external non-user
activities like this.


#13

Hmm, I thought that we use implicit transactions, thus audit is inserted before actual change on audited record is done, but it looks like audited gem (or Rails hooks) is somehow creating explicit transactions:

[app|I||] Started DELETE "/bookmarks/7-running" for 127.0.0.1 at 2018-11-16 16:59:57 +0100
[app|I|7f0|c63] Processing by BookmarksController#destroy as HTML
[app|D|7f0|c63]   Parameters: {"authenticity_token"=>"Cn9Orxs/q6/nI4VH9H5wh/BkP90h8Nnh/4NXQQ/whNkck80WrR7mKgGna09inJkL4DzP+B1eBEhAHEw9Ze4IoQ==", "id"=>"7-running"}
[sql|D|7f0|c63]   User Load (0.2ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = ? LIMIT ?  [["id", 4], ["LIMIT", 1]]
[sql|D|7f0|c63]   ↳ app/controllers/concerns/foreman/controller/authentication.rb:10
[sql|D|7f0|c63]   AuthSource Load (0.3ms)  SELECT  "auth_sources".* FROM "auth_sources" WHERE "auth_sources"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
[sql|D|7f0|c63]   ↳ app/models/user.rb:175
[app|I|7f0|c63] Current user set to admin (admin)
[app|D|7f0|c63] Current location set to none
[app|D|7f0|c63] Current organization set to none
[sql|D|7f0|c63]    (0.2ms)  SELECT  "taxonomies"."id" FROM "taxonomies" WHERE "taxonomies"."type" IN ('Location') LIMIT ?  [["LIMIT", 1]]
[sql|D|7f0|c63]   ↳ lib/core_extensions.rb:111
[sql|D|7f0|c63]    (0.2ms)  SELECT  "taxonomies"."id" FROM "taxonomies" WHERE "taxonomies"."type" IN ('Organization') LIMIT ?  [["LIMIT", 1]]
[sql|D|7f0|c63]   ↳ lib/core_extensions.rb:111
[sql|D|7f0|c63]   Bookmark Exists (0.2ms)  SELECT  1 AS one FROM "bookmarks" WHERE (((bookmarks.public = 't') OR (bookmarks.owner_id = 4 AND bookmarks.owner_type = 'User'))) LIMIT ?  [["LIMIT", 1]]
[sql|D|7f0|c63]   ↳ app/controllers/concerns/find_common.rb:12
[sql|D|7f0|c63]   Bookmark Load (0.1ms)  SELECT  "bookmarks".* FROM "bookmarks" WHERE (((bookmarks.public = 't') OR (bookmarks.owner_id = 4 AND bookmarks.owner_type = 'User'))) AND "bookmarks"."id" = ? ORDER BY "bookmarks"."name" ASC LIMIT ?  [["id", 7], ["LIMIT", 1]]
[sql|D|7f0|c63]   ↳ app/models/concerns/parameterizable.rb:30
[sql|D|7f0|c63]    (0.0ms)  begin transaction
[sql|D|7f0|c63]   ↳ app/controllers/bookmarks_controller.rb:24
[sql|D|7f0|c63]   Bookmark Load (0.1ms)  SELECT  "bookmarks".* FROM "bookmarks" WHERE "bookmarks"."id" = ? ORDER BY "bookmarks"."name" ASC LIMIT ?  [["id", 7], ["LIMIT", 1]]
[sql|D|7f0|c63]   ↳ app/models/concerns/audit_extensions.rb:187
[sql|D|7f0|c63]    (0.3ms)  SELECT MAX("audits"."version") FROM "audits" WHERE "audits"."auditable_id" = ? AND "audits"."auditable_type" = ?  [["auditable_id", 7], ["auditable_type", "Bookmark"]]
[sql|D|7f0|c63]   ↳ app/controllers/bookmarks_controller.rb:24
[sql|D|7f0|c63]   Audited::Audit Create (0.3ms)  INSERT INTO "audits" ("auditable_id", "auditable_type", "user_id", "username", "action", "audited_changes", "version", "request_uuid", "created_at", "remote_address", "auditable_name") VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)  [["auditable_id", 7], ["auditable_type", "Bookmark"], ["user_id", 4], ["username", "Admin User"], ["action", "destroy"], ["audited_changes", "---\nname: running\nquery: state = running\ncontroller: foreman_tasks_tasks\npublic: true\nowner_id: 1\nowner_type: User\n"], ["version", 1], ["request_uuid", "c63ad336-974d-4c7a-a978-6e95edbb17f3"], ["created_at", "2018-11-16 15:59:57.100903"], ["remote_address", "127.0.0.1"], ["auditable_name", "running"]]
[sql|D|7f0|c63]   ↳ app/controllers/bookmarks_controller.rb:24
[aud|I|7f0|c63] Bookmark (7) destroy event on name running
[aud|I|7f0|c63] Bookmark (7) destroy event on query state = running
[aud|I|7f0|c63] Bookmark (7) destroy event on controller foreman_tasks_tasks
[aud|I|7f0|c63] Bookmark (7) destroy event on public true
[aud|I|7f0|c63] Bookmark (7) destroy event on owner_id 1
[aud|I|7f0|c63] Bookmark (7) destroy event on owner_type User
[sql|D|7f0|c63]   Bookmark Destroy (0.2ms)  DELETE FROM "bookmarks" WHERE "bookmarks"."id" = ?  [["id", 7]]
[sql|D|7f0|c63]   ↳ app/controllers/bookmarks_controller.rb:24
[sql|D|7f0|c63]    (184.7ms)  commit transaction
[sql|D|7f0|c63]   ↳ app/controllers/bookmarks_controller.rb:24
[app|I|7f0|c63] Redirected to http://localhost:3000/bookmarks
[app|I|7f0|c63] Completed 302 Found in 236ms (ActiveRecord: 187.3ms)

My proposal is async and one way only, creating callbacks when an event started does not make much sense to me. You can’t cancel it, you don’t know really if it was already processed or not.