Create event hook infrastructure #952

vuntz · 2017-01-04T10:39:04Z

There are several cases where we have events that should trigger some activity in some other part of Crowbar. For instance:

when the public name of a node is saved, it may impact the endpoint of an OpenStack service
when the public name of the VIP of haproxy is changed, it impacts the endpoint of OpenStack services
when the keystone proposal is applied, we may want to reapply all proposals that depend on keystone

What we need for this is the ability to notify about the events in the rails application and then to dispatch the notifications to hooks listen listening to them that will decide if some action should be triggered.

The main reason we didn't have this in the past is that we likely don't want to do that in the foreground of the rails application. But now that we have delayed_job, we can send the notifications and run the hooks in the background.

In this commit, we add the simple infrastructure about notifications and hooks:

the events are defined with a name and a hash that contains the details of the event. The structure of the hash depends on the event.
we have two events that are generated and notified: node_changed (when a node is saved and some specific attributes have been changed) and proposal_applied (when a proposal is successfully applied).
a simple dispatcher exists that simply connects the hooks to the events.
the hooks only exist for service objects for the time being; a service object simply needs to have a event_hook method to register the hook, and will need to filter for the events it cares about. The signature of event_hook is as follows: def event_hook(role, event, details)

It could be argued that the hooks should be registered for some specific events (hence moving the filter to the event dispatcher), but it's not worth the complexity for now.

We could add many more events but for the time being, we only add the events that we know are useful.

houndci-bot · 2017-01-04T10:39:10Z

crowbar_framework/lib/crowbar/notify.rb

+      def trigger_hooks(event, details)
+        BarclampCatalog.barclamps.keys.each do |barclamp|
+          begin
+            cls = eval("#{barclamp.camelize}Service")


Lint/Eval: The use of eval is a serious security risk.

matelakat

Thanks for the clarification! Please see my comment about the @notify_on_save hash.

matelakat · 2017-01-04T11:19:18Z

crowbar_framework/app/models/node_object.rb

@@ -259,6 +259,7 @@ def initialize(node)
    end
    # deep clone of @role.default_attributes, used when saving node
    @attrs_last_saved = @role.default_attributes.deep_dup
+    @notify_on_save = {}


This variable really holds the notifications to be sent on the next save, if understand it correctly. Also looking at the point where the notifications are sent out, it seems, that we are not using the value in the hash, only the keys. I think a better name for this variable might help, and also making it an array/set would also help. I would name it something like: on_save_events. Thus the protocol would look like: the client adds/queues up items to the on_save_events collection, and those are being sent and flushed on save.

matelakat · 2017-01-04T11:21:33Z

crowbar_framework/app/models/node_object.rb

@@ -479,6 +483,7 @@ def validate_public_name(value, unique_check = true)
  def update_public_name(value)
    unless value.nil?
      crowbar["crowbar"]["public_name"] = value
+      @notify_on_save["public_name"] = true


It might be a stupid question: Are we sure that update_public_name is only called whenever the value of it has changed? I'm not seeing the same check-if-changed mechanism as earlier.

See https://github.com/crowbar/crowbar-core/pull/952/files#diff-496ddf4ffc5694a5fcdddbff8206dc8dR441 (there's also force_public_name= where the check is missing, but I think it's fine to still send the event in that case since it's forced).

matelakat · 2017-01-04T11:22:42Z

crowbar_framework/app/models/node_object.rb

+
+      details = {
+        node: @node.name,
+        attributes: @notify_on_save.keys


As I mentioned earlier, we only send the keys, not the values. So we might want to send the values as well, or use a single set as mentioned earlier.

I can move to a set, sure. FWIW, in ruby, a set is implemented (behind the scenes) exactly the way I did, so won't be too much different but I guess it's slightly more readable.

houndci-bot · 2017-01-04T12:20:52Z

crowbar_framework/lib/crowbar/event_dispatcher.rb

+      def trigger_hooks(event, details)
+        BarclampCatalog.barclamps.keys.each do |barclamp|
+          begin
+            cls = eval("#{barclamp.camelize}Service")


Lint/Eval: The use of eval is a serious security risk.

Calculating the class name and obtaining the class surely belongs in methods in BarclampCatalog. Doing it here sounds like privacy violation / feature envy.

Isnt the way of doing this better with Kernel.const_get ?*

*not a ruby expert

@Itxaka you're right.

We actually have ServiceObject.get_service exactly for this...

vuntz · 2017-01-04T12:21:56Z

@matelakat I addressed your comments. I also renamed the small library to EventDispatcher as Notify was too generic...

aspiers · 2017-01-04T13:17:03Z

crowbar_framework/lib/crowbar/event_dispatcher.rb

+          end
+        end
+      end
+      handle_asynchronously :trigger_hooks


Should be a blank line before this.

Oh, maybe not. It's kind of like a decorator.

vuntz · 2017-01-04T13:21:23Z

crowbar_framework/lib/crowbar/event_dispatcher.rb

+          proposals.each do |proposal|
+            role = proposal.role
+            next if role.nil?
+            service.event_hook(role, event, details)


We need some exception catching here.

matelakat · 2017-01-04T14:06:29Z

crowbar_framework/app/models/node_object.rb

@@ -289,7 +291,10 @@ def availability_zone
  def availability_zone=(value)
    @node["crowbar_wall"] ||= {}
    @node["crowbar_wall"]["openstack"] ||= {}
-    @node["crowbar_wall"]["openstack"]["availability_zone"] = value
+    if @node["crowbar_wall"]["openstack"]["availability_zone"] != value


@vuntz as I mentioned it in the "eliminate saves" PR earlier, this pattern might worth extracting to a helper, something like: set_value_and_call_function_if_changed or I believe ruby must have better ways to express it.

toabctl · 2017-01-04T14:47:39Z

crowbar_framework/app/models/node_object.rb

+      }
+      Crowbar::EventDispatcher.trigger_hooks(:node_changed, details)
+
+      @on_save_events = Set.new


Can this be called in parallel by different puma threads? If so isn't this a race and we could miss save events when cleaning the Set?

The node object is not shared among puma threads, so it should be fine. When we create threads ourselves, we should be careful of course and use proper locking mechanisms, but it's not specific to this change.

aspiers

Once the variable is renamed and comment addressed, I'm good with this PR.

aspiers · 2017-01-19T18:08:16Z

crowbar_framework/app/models/node_object.rb

@@ -259,6 +260,7 @@ def initialize(node)
    end
    # deep clone of @role.default_attributes, used when saving node
    @attrs_last_saved = @role.default_attributes.deep_dup
+    @attributes_changed_for_event = Set.new


As discussed could you rename this to @attributes_changed and then add a comment explaining what it's used for?

aspiers · 2017-01-19T18:09:53Z

Well, of course the merge conflict and Travis failures have to be fixed first :)

There are several cases where we have events that should trigger some activity in some other part of Crowbar. For instance: - when the public name of a node is saved, it may impact the endpoint of an OpenStack service - when the public name of the VIP of haproxy is changed, it impacts the endpoint of OpenStack services - when the keystone proposal is applied, we may want to reapply all proposals that depend on keystone What we need for this is the ability to notify about the events in the rails application and then to dispatch the notifications to hooks listen listening to them that will decide if some action should be triggered. The main reason we didn't have this in the past is that we likely don't want to do that in the foreground of the rails application. But now that we have delayed_job, we can send the notifications and run the hooks in the background. In this commit, we add the simple infrastructure about notifications and hooks: - the events are defined with a name and a hash that contains the details of the event. The structure of the hash depends on the event. - we have two events that are generated and notified: node_changed (when a node is saved and some specific attributes have been changed) and proposal_applied (when a proposal is successfully applied). - a simple dispatcher exists that simply connects the hooks to the events. - the hooks only exist for service objects for the time being; a service object simply needs to have a event_hook method to register the hook, and will need to filter for the events it cares about. The signature of event_hook is as follows: def event_hook(role, event, details) It could be argued that the hooks should be registered for some specific events (hence moving the filter to the event dispatcher), but it's not worth the complexity for now. We could add many more events but for the time being, we only add the events that we know are useful.

vuntz · 2017-01-26T09:22:18Z

@aspiers rebased

Rebase of PR crowbar#952 Only committing the infrastructure changes For usage example(s), see: crowbar#952 crowbar/crowbar-ha#171 crowbar/crowbar-openstack#717 There are several cases where we have events that should trigger some activity in some other part of Crowbar. For instance: - when the public name of a node is saved, it may impact the endpoint of an OpenStack service - when the public name of the VIP of haproxy is changed, it impacts the endpoint of OpenStack services - when the keystone proposal is applied, we may want to reapply all proposals that depend on keystone What we need for this is the ability to notify about the events in the rails application and then dispatch the notifications to hooks which listen for them and decide if some action should be triggered. The main reason we didn't have this in the past is that we likely don't want to do that in the foreground of the rails application. But now that we have delayed_job, we can send the notifications and run the hooks in the background. In this commit, we add the simple infrastructure about notifications and hooks: - the events are defined with a name and a hash that contains the details of the event. The structure of the hash depends on the event. - a simple dispatcher exists that simply connects the hooks to the events. - the hooks only exist for service objects for the time being; a service object simply needs to have a event_hook method to register the hook, and will need to filter for the events it cares about. The signature of event_hook is as follows: def event_hook(role, event, details) It could be argued that the hooks should be registered for some specific events (hence moving the filter to the event dispatcher), but it's not worth the complexity for now.

Rebase of PR crowbar#952 but only including the new infrastructure class For usage examples, see: crowbar#952 crowbar/crowbar-ha#171 crowbar/crowbar-openstack#717 There are several cases where we have events that should trigger some activity in some other part of Crowbar. For instance: - when the public name of a node is saved, it may impact the endpoint of an OpenStack service - when the public name of the VIP of haproxy is changed, it impacts the endpoint of OpenStack services - when the keystone proposal is applied, we may want to reapply all proposals that depend on keystone What we need for this is the ability to notify about the events in the rails application and then dispatch the notifications to hooks which listen for them and decide if some action should be triggered. The main reason we didn't have this in the past is that we likely don't want to do that in the foreground of the rails application. But now that we have delayed_job, we can send the notifications and run the hooks in the background. In this commit, we add the simple infrastructure about notifications and hooks: - the events are defined with a name and a hash that contains the details of the event. The structure of the hash depends on the event. - a simple dispatcher exists that simply connects the hooks to the events. - the hooks only exist for service objects for the time being; a service object simply needs to have a event_hook method to register the hook, and will need to filter for the events it cares about. The signature of event_hook is as follows: def event_hook(role, event, details) It could be argued that the hooks should be registered for some specific events (hence moving the filter to the event dispatcher), but it's not worth the complexity for now.

vuntz · 2017-10-09T08:14:38Z

Completed in #1308

vuntz added the wip label Jan 4, 2017

vuntz assigned aspiers and rsalevsky Jan 4, 2017

houndci-bot reviewed Jan 4, 2017

View reviewed changes

vuntz mentioned this pull request Jan 4, 2017

Example of how to use event hook infra crowbar/crowbar-openstack#717

Open

matelakat suggested changes Jan 4, 2017

View reviewed changes

vuntz force-pushed the event-hooks branch 2 times, most recently from 66792a7 to 2ee29ec Compare January 4, 2017 12:20

houndci-bot reviewed Jan 4, 2017

View reviewed changes

aspiers reviewed Jan 4, 2017

View reviewed changes

vuntz commented Jan 4, 2017

View reviewed changes

matelakat reviewed Jan 4, 2017

View reviewed changes

vuntz force-pushed the event-hooks branch from 2ee29ec to bc37a50 Compare January 4, 2017 14:15

dirkmueller added this to the Cloud 7 Update1 milestone Jan 4, 2017

toabctl reviewed Jan 4, 2017

View reviewed changes

vuntz force-pushed the event-hooks branch 2 times, most recently from 1fee9e4 to 83ad9a3 Compare January 4, 2017 15:29

vuntz mentioned this pull request Jan 4, 2017

crowbar: Generate an event when a cluster config is changed crowbar/crowbar-ha#171

Open

aspiers approved these changes Jan 19, 2017

View reviewed changes

vuntz force-pushed the event-hooks branch from 83ad9a3 to 93b2984 Compare January 26, 2017 09:22

aspiers removed their assignment Feb 5, 2017

aspiers requested a review from rsalevsky February 5, 2017 22:53

aspiers assigned vuntz and unassigned rsalevsky Feb 5, 2017

rsalevsky approved these changes Feb 6, 2017

View reviewed changes

jsuchome added the needs backport to SOC7 (stable/4.0) label Jun 15, 2017

scottwulf mentioned this pull request Aug 23, 2017

Create event hook infrastructure #1308

Merged

scottwulf mentioned this pull request Aug 30, 2017

[4.0] Create event hook infrastructure #1313

Merged

dirkmueller removed the needs backport to SOC7 (stable/4.0) label Oct 5, 2017

vuntz closed this Oct 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create event hook infrastructure #952

Create event hook infrastructure #952

vuntz commented Jan 4, 2017 •

edited by aspiers

houndci-bot Jan 4, 2017

matelakat left a comment

matelakat Jan 4, 2017

matelakat Jan 4, 2017

vuntz Jan 4, 2017

matelakat Jan 4, 2017

vuntz Jan 4, 2017

houndci-bot Jan 4, 2017

aspiers Jan 4, 2017

Itxaka Jan 4, 2017

vuntz Jan 4, 2017

vuntz Jan 4, 2017

vuntz commented Jan 4, 2017

aspiers Jan 4, 2017

aspiers Jan 4, 2017

vuntz Jan 4, 2017

matelakat Jan 4, 2017

toabctl Jan 4, 2017

vuntz Jan 4, 2017

aspiers left a comment

aspiers Jan 19, 2017

aspiers commented Jan 19, 2017

vuntz commented Jan 26, 2017

vuntz commented Oct 9, 2017

Create event hook infrastructure #952

Create event hook infrastructure #952

Conversation

vuntz commented Jan 4, 2017 • edited by aspiers

Choose a reason for hiding this comment

matelakat 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

vuntz commented Jan 4, 2017

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

aspiers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aspiers commented Jan 19, 2017

vuntz commented Jan 26, 2017

vuntz commented Oct 9, 2017

vuntz commented Jan 4, 2017 •

edited by aspiers