Event parser can parse new format of target #160

Ladas · 2017-03-03T12:02:42Z

Depends on:

Target and target collection abstractions manageiq#14249

Event parser can parse new format of target + specs

Ladas · 2017-03-03T12:12:03Z

@durandom @kbrock @agrare @blomquisg @Fryguy so this is how the new target format could look like, so we don;t require a Target being created in our DB

{"Vm" => {:manager_id => ems_id, :manager_ref => Set.new({:ems_ref => xy}, {:ems_ref => zz}) }}

It would be probably worth to pack this to ManagerRefresh::Target, where .serialize would return the ^ hash, so it can be queued to the refresh and we could then get it back with ManagerRefresh::Target.new(class, manager_id:, manager_ref: , ..)

then you could always get the records connected to the Target with:
target.manager_ref.map { |ref| target.class.find_by(ref.merge(:ems_id => target.manager_id)) }

or any other way, which would be more effective, though we should not need to really instantiate it ourselves

durandom

I think it would be great if you could extract the parsing stuff into its own class.

For starters it could live here and be called Target although this is probably not the final name.

Then you can write a spec for that object and not convolute the other specs

durandom · 2017-03-07T08:38:30Z

spec/models/manageiq/providers/amazon/cloud_manager/event_parser_spec.rb

+    _guid, _server, zone = EvmSpecHelper.create_guid_miq_server_zone
+    @ems                 = FactoryGirl.create(:ems_amazon, :zone => zone)
+
+    allow_any_instance_of(EmsEvent).to receive(:handle_event)


allow_any_instance_of is the root of all evil. Chris and JoeR try hard to get eliminate it. Can do without it? Maybe inject an instance of EmsEvent into somewhere?

sure we can get rid of that if we will rewrite the code. The EmsEvent.add needs to have submethods, now it's all entangled together.

durandom · 2017-03-07T08:52:57Z

app/models/manageiq/providers/amazon/cloud_manager/event_parser.rb

+
+    case event.full_data["event_source"]
+    when :cloud_watch_api
+      collect_cloudwatch_api_references!(available_targets, event.full_data.fetch_path("detail", "requestParameters") || {})


all those bang methods. It's hard to tell what they actually modify.
This screams for extraction of an object 🛠

well they do build available_targets

durandom · 2017-03-07T08:56:10Z

app/models/manageiq/providers/amazon/cloud_manager/event_parser.rb

+      raise "Unsupported event source #{event["event_source"]}"
+    end
+
+    send("parse_#{event["event_source"]}_event!", event, event_hash)


chilly bang bang. What is modified by this method?
It would be much easier to follow if you return the modified version and assign it.

event_hash = send("parse_#{event["event_source"]}_event", event)

but I just saw, this is already in the code :/

yeah this would be probably nicer

durandom · 2017-03-07T09:09:17Z

I like the .serialize idea 👍

Could you explain a bit, when and how this logic is triggered?
So, when is a refresh scheduled for such a non existent target?

Ladas · 2017-03-07T13:57:30Z

@durandom so this logic is not trigered yet, but the workflow should be

parse event so we can store EmsEvent to the DB
if there is a targeted refresh, queue for refresh all targets we get from parser_class.parse_targets(ems_event) and optionally limit the targets parsed

so the new automate method would be something like:
2.1) /System/event_handlers/graph_refresh # queues all available targets for refresh
2.2) /System/event_handlers/graph_refresh?limit_targets=Vm # queues only target of type Vm, if available
2.3) /System/event_handlers/graph_refresh?with_relations=Vm # queues Vm targeted refresh, with options to scan the Vm and queue also all it's relations for refresh, so it's a Vm full refresh

Save EmsEvent with all the targets as M:N relations, this needs to be invoked after the refresh, so all the targets are in the DB

Adding class for parsing targets from an Event

Adding specs for EmsEvent Target parsing

durandom · 2017-03-08T07:42:17Z

@Ladas 🤘 for creating all in new classes

durandom

great iteration

next step should be the EventTarget class extraction

And I still need to understand how this will be used when picked later from the event queue

durandom · 2017-03-08T07:48:25Z

spec/models/manageiq/providers/amazon/cloud_manager/event_target_parser_spec.rb

+
+  context "AWS Config Event" do
+    it "parses vm_ems_ref into event" do
+      ems_event = create_ems_event("sqs_message.json")


Still, I would create an EmsEvent via a factory here and not test the whole EmsEvent Stream stack.

But I get your integration spec needs. But shouldnt those parts of the code already be tested somewhere else?

I really want to test this on real events, so using the actual jsons of the event. I don't really want to stub the middlelayers. I want to add like 40-80 main events for testing, would be dull and error prone to do the double stubbing

durandom · 2017-03-08T07:54:46Z

app/models/manageiq/providers/amazon/cloud_manager/event_target_parser.rb

@@ -0,0 +1,139 @@
+class ManageIQ::Providers::Amazon::CloudManager::EventTargetParser


👍 for this class

Now extract another class, e.g. EventTarget which holds add_reference and the available_targets data. Then you have fully encapsulated that data you want to transport into a class and moved away from hashes.

done, ManagerRefresh::Target and ManagerRefresh::TargetCollection are being introduced as a dependency

Change EventTargetParser to use the ManagerRefresh target abstractions

Use ManagerRefresh::TargetCollection instead of the Amazon one

kbrock

this is looking nice.

I got the general idea behind this. My comments are only cosmetic

kbrock · 2017-03-09T19:15:22Z

app/models/manageiq/providers/amazon/cloud_manager/event_target_parser.rb

+  def collect_config_references!(target_collection, event_data)
+    resource_type = event_data.fetch_path("configurationItem", "resourceType")
+    resource_id   = event_data.fetch_path("configurationItem", "resourceId")
+    target_class  = case resource_type


would it make sense to do a hash lookup here?

possibly, but maybe the case is enough? Should be still pretty quick

kbrock · 2017-03-09T19:20:44Z

app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb

    end
+    byebug


kbrock · 2017-03-09T19:27:57Z

app/models/manageiq/providers/amazon/cloud_manager/event_target_parser.rb

+      collect_cloudwatch_api_references!(target_collection, x, depth + 1)
+    end
+    (event_data.fetch_path("instances") || []).each do |x|
+      collect_cloudwatch_api_references!(target_collection, x, depth + 1)


would it make sense to expand this api? collect_cloud_watch_api_referenceses!

of course, naming is hard. Then again, do you ever collect_cloudwatch_api_references with a single array?

collect_cloud_watch_api_referenceses!(target_collection, event_data.fetch_path("instances") || [], depth + 1)

no usually the cloudwatch events have a nested arrays that contain references

kbrock · 2017-03-09T19:28:23Z

app/models/manageiq/providers/amazon/inventory/collector.rb

@@ -30,49 +30,21 @@ def initialize(_manager, _target)

  def initialize_inventory_sources
    @instances                   = []
-    @instances_refs              = Set.new


yay.

I'm assuming this doesn't add any O(N) lookups

the refs were moved to ManagerRefresh::TargetCollection

Ooh, then that is very cool

kbrock · 2017-03-09T19:30:11Z

app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb

  end

  def infer_related_ems_refs!
    # We have a list of instances_refs collected from events. Now we want to look into our DB and API, and collect
    # ems_refs of every related object. Now this is not very nice fro ma design point of view, but we really want
    # to see changes in VM's associated objects, so the VM view is always consistent and have fresh data. The partial
    # reason for this is, that AWS doesn't send all the objects state change,
-    changed_vms = manager.vms.where(:ems_ref => instances_refs.to_a).includes(:key_pairs, :network_ports, :floating_ips,
-                                                                          :orchestration_stack)
+    byebug


kbrock · 2017-03-09T19:31:28Z

app/models/manageiq/providers/amazon/inventory/persister/target_collection.rb

@@ -24,6 +24,10 @@ def initialize_inventory_collections

  private

+  def references(collection)


is this a generic method?
Can you get define it in somewhere like ManagerRefresh::Inventory::Collector or highter?

right, it's not really generic since it states :ems_ref, but maybe I could pack part of it to the TargetCollection

…Persister Use ManagerRefresh::TargetCollection collection for TargetCollection Persister

…Collector Use ManagerRefresh::TargetCollection collection for TargetCollection Collector

add_target! method was renamed to add_target

Fix EventTargetParser specs, it's broken due to core changes

miq-bot · 2017-03-13T10:32:35Z

Some comments on commits Ladas/manageiq-providers-amazon@308e47b~...50ee99e

spec/models/manageiq/providers/amazon/cloud_manager/event_target_parser_spec.rb

⚠️ - 6 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2017-03-13T10:32:39Z

Checked commits Ladas/manageiq-providers-amazon@308e47b~...50ee99e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 0 offenses detected
Everything looks good. 👍

durandom

👍 except for some refactoring suggestions

durandom · 2017-03-13T13:00:57Z

app/models/manageiq/providers/amazon/cloud_manager/event_target_parser.rb

+  #
+  # @return [Array] Array of ManagerRefresh::Target objects
+  def parse
+    parse_ems_event_targets(ems_event)


Now that you add ems_event as a state (instance variable), I think you can also create target_collection as an instance var and let collect_cloudwatch_api_references operate on that.

durandom · 2017-03-13T13:01:45Z

app/models/manageiq/providers/amazon/inventory/collector.rb

-  attr_reader :load_balancers, :load_balancers_refs, :load_balancers_deleted
-  attr_reader :stacks, :stacks_refs, :stacks_deleted
-  attr_reader :cloud_volumes, :cloud_volumes_refs
-  attr_reader :cloud_volume_snapshots, :cloud_volume_snapshots_refs


👏 for less attr_readers

durandom · 2017-03-13T13:03:01Z

app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb

@@ -3,24 +3,30 @@ def initialize(_manager, _target)
    super
    parse_targets!
    infer_related_ems_refs!
+
+    target.manager_refs_by_association_reset


This would be a perfect ! method

durandom · 2017-03-13T13:04:40Z

app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb

  def parse_vm_target!(t)
-    instances_refs << t.ems_ref if t.ems_ref
+    target.add_target(:association => :vms, :manager_ref => {:ems_ref => t.ems_ref}) if t.ems_ref


If this is mostly always target.add_target, you might also allow target.add syntax

How about renaming this to targets or target_collection

Ladas assigned durandom Mar 3, 2017

Ladas added enhancement euwe/no labels Mar 3, 2017

durandom changed the title ~~Event parser can parse new format of target~~ [WIP] Event parser can parse new format of target Mar 7, 2017

durandom added the wip label Mar 7, 2017

durandom suggested changes Mar 7, 2017

View reviewed changes

Ladas added 2 commits March 7, 2017 17:43

Adding class for parsing targets from an Event

308e47b

Adding class for parsing targets from an Event

Adding specs for EmsEvent Target parsing

a84469f

Adding specs for EmsEvent Target parsing

Ladas force-pushed the event_parser_can_parse_new_format_of_target branch from c7a8076 to a84469f Compare March 7, 2017 16:44

durandom reviewed Mar 8, 2017

View reviewed changes

Ladas added 2 commits March 9, 2017 17:51

Change EventTargetParser to use the ManagerRefresh target abstractions

49bd888

Change EventTargetParser to use the ManagerRefresh target abstractions

Use ManagerRefresh::TargetCollection instead of the Amazon one

4288763

Use ManagerRefresh::TargetCollection instead of the Amazon one

kbrock reviewed Mar 9, 2017

View reviewed changes

Ladas added 2 commits March 10, 2017 09:25

Use ManagerRefresh::TargetCollection collection for TargetCollection …

8126027

…Persister Use ManagerRefresh::TargetCollection collection for TargetCollection Persister

Use ManagerRefresh::TargetCollection collection for TargetCollection …

f67259d

…Collector Use ManagerRefresh::TargetCollection collection for TargetCollection Collector

Ladas force-pushed the event_parser_can_parse_new_format_of_target branch from 2d68ec5 to f67259d Compare March 10, 2017 08:26

add_target! method was renamed to add_target

7eed07f

add_target! method was renamed to add_target

Ladas closed this Mar 10, 2017

Ladas reopened this Mar 10, 2017

Fix EventTargetParser specs

50ee99e

Fix EventTargetParser specs, it's broken due to core changes

Ladas changed the title ~~[WIP] Event parser can parse new format of target~~ Event parser can parse new format of target Mar 13, 2017

Ladas removed the wip label Mar 13, 2017

durandom approved these changes Mar 13, 2017

View reviewed changes

durandom merged commit 578c455 into ManageIQ:master Mar 13, 2017

durandom added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 13, 2017

		@@ -0,0 +1,139 @@
		class ManageIQ::Providers::Amazon::CloudManager::EventTargetParser

		@@ -24,6 +24,10 @@ def initialize_inventory_collections

		private

		def references(collection)

Event parser can parse new format of target #160

Event parser can parse new format of target #160

Conversation

Ladas commented Mar 3, 2017 • edited Loading

Ladas commented Mar 3, 2017 • edited Loading

durandom 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

durandom commented Mar 7, 2017

Ladas commented Mar 7, 2017

durandom commented Mar 8, 2017

durandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 13, 2017

miq-bot commented Mar 13, 2017

durandom 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

Ladas commented Mar 3, 2017 •

edited

Loading

Ladas commented Mar 3, 2017 •

edited

Loading

Ladas Mar 9, 2017 •

edited

Loading