Ems event collection refresh #105

Ladas · 2017-01-13T16:45:49Z

A targeted refresh based on AwsConfig events. Creating an EmsEventCollection that is used for grouping EmsEvent targets and refreshing them in one graph Refresh. This way, we are able to save all needed cross-links, in the case many AWS object are created at once (e.g. Vm, Interface, EIP, SecurityGroup, etc.)

This targeted refresh doesn't do any API call, it uses Event Payload data to refresh MIQ DB.

Implements:
https://www.pivotaltracker.com/story/show/137733251

gberginc

I've got some basic comments/questions since the changes in these PR also affect the volume parser.

Overall I see a "massive" overhaul of the refreshers. Is there any external document I could have a look to better understand the idea behind these changes?

gberginc · 2017-01-16T15:58:41Z

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

@@ -143,6 +147,7 @@ def parse_flavor(flavor)

    {
      :type                     => ManageIQ::Providers::Amazon::CloudManager::Flavor.name,
+      :ext_management_system    => ems,


Where is this being used?

The ems relation is usually filled automatically, when it's saved like ems.association.build, but for a targeted refresh, we don't build it like this, so we need to provide it.

gberginc · 2017-01-16T16:41:53Z

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

+        if inventory.kind_of?(ManageIQ::Providers::Amazon::Inventory::Targets::EmsEventCollection)
+          # For EmsEventCollection, we want to parse everything, regardless whether it belongs to Cloud or Network
+          # Manager
+          ManageIQ::Providers::Amazon::NetworkManager::RefreshParserInventoryObject.new(inventory).populate_inventory_collections


Here is where we'll have to populate other refresh parsers as well?

Events are going only through a cloud manager now, so no need.

Ah, yeah, sorry, I answered to something else it seems. Yes. In here, it should parse all data, if this was an event based refresh.

gberginc · 2017-01-16T16:48:36Z

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

+  end
+
+  def datetime_min
+    targets.collect(&:timestamp).min - 3.minutes


I suggest adding a comment why 3 mins are used here.

it's a first draft condition to find related events, usually the similar events should have a CloudTrail id, but it's not true for some reason, when deploying through stack.

gberginc · 2017-01-16T16:51:28Z

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

+  def all_related_ems_events(ems)
+    # We want all EmsEvents around the time of collected EmsEvents, and to the future since AWS send all related events
+    # at once
+    EmsEvent.where(:ems_id => ems.id).where(:timestamp => datetime_min..DateTime::Infinity.new).order("timestamp DESC")


Is there any nicer way for checking timestamp >= datetime_min?

yes, using arel_table

gberginc · 2017-01-16T16:52:34Z

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

@@ -23,7 +27,7 @@ def add_inventory_collections(inventory_collections, inventory_collections_data

  def add_remaining_inventory_collections(inventory_collections_data)
    # Get names of all inventory collections defined in InventoryCollectionDefaultInitData
-    all_inventory_collections = ManageIQ::Providers::Amazon::Inventory::InventoryCollectionDefaultInitData.
+    all_inventory_collections     = ManageIQ::Providers::Amazon::Inventory::InventoryCollectionDefaultInitData.


Are the spaces intentional?

hm no, this one is a leftover, not caught by auto-align for some reason :-)

aiperon · 2017-01-17T00:34:40Z

app/models/manageiq/providers/amazon/inventory/targets/ems_event_collection.rb

+      uniq_checker[unique_checker_id] = true
+
+      event_payload   = event_payload(ems_event)
+      collection_name = case resource_type


Should we also expect AWS::S3 events here?

Yes, we are going to add storage events, but I was planning to add them after we have at least the basic logic working.

yeah, we should, haven't played with those yet

Ladas · 2017-01-17T09:34:13Z

@aiperon @gberginc btw. this whole feature is hidden under a configuration now and it's possible, we will end up not using it :-(

So benefit of this is, that we can refresh our data, just based on the payload of the events we receive, so we do not need to make any API calls.

But a disadvantage of this is that AwsConfig events are slow, it takes like 5-10 minutes for them to arrive. And also not all services are covered by them, I don't get events for the whole CloudFormations, images, etc, so we still need to make a periodical full refresh.

So I will be exploring CloudWatch events next, where we will need to make API requests, since there is not a full payload sent and we will need to batch them, to not hit API limits. But they are very quick in turn, we receive them almost instantly.

gberginc · 2017-01-17T09:38:23Z

With "this whole feature" you mean event collection refresh or the entire inventory that was introduced in #102?

We'll focus on other aspects of storage managers now until we better understand how to use events.

Ladas · 2017-01-17T12:33:08Z

@gberginc just this PR with refresh based on Events payload. Right now, each event will cause a full refresh of a CloudManager (including the storage events) and after that NetworkManager refresh is queued, so StorageManagers should be too (here: https://github.com/Ladas/manageiq-providers-amazon/blob/81fcc78bf015e6fbb706e9afe069afa4d6331769/app/models/manageiq/providers/amazon/cloud_manager/refresher.rb#L69-L69)

So yeah, ignore events for now, as by default, storage events are already caught.

miq-bot · 2017-01-17T12:42:29Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-01-18T17:32:45Z

This pull request is not mergeable. Please rebase and repush.

durandom · 2017-01-20T11:21:17Z

Does this PR depend on ManageIQ/manageiq#13408 ?
And could you describe what this PR does in the description? It's massive!

Remove single event payload refresh

Add EmsEventCollection target allowing us to refresh multiple avents at once

Add ems_event_collection collection wrapper allowing us to group EmsEvents and collect all related events.

Add attr_readers to inventory for each source of data

Group all ems events into EmsEventCollection and parse then regardless if they come from Cloud or Network Manager.

Add EmsEventCollection target to factory

Enhance Inventory::Targets base class

Move the array filtering to parser side, inventory side should have only API filtering

Fix rubocop issues

miq-bot · 2017-01-20T11:42:55Z

Checked commits Ladas/manageiq-providers-amazon@2fff138~...9c9273b with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
7 files checked, 45 offenses detected

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

❕ - Line 33, Col 121 - Metrics/LineLength - Line is too long. [129/120]
❕ - Line 51, Col 121 - Metrics/LineLength - Line is too long. [142/120]

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

❕ - Line 35, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for initialize_inventory_sources is too high. [45/20]
❕ - Line 35, Col 3 - Metrics/MethodLength - Method has too many lines. [45/25]

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

❗ - Line 31, Col 7 - Style/MultilineMethodCallIndentation - Align .public_instance_methods with ManageIQ::Providers::Amazon::Inventory::InventoryCollectionDefaultInitData on line 30.

app/models/manageiq/providers/amazon/inventory/targets/ems_event_collection.rb

❗ - Line 100, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 100, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 109, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 109, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 113, Col 57 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 114, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 114, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 123, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 123, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 127, Col 55 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 128, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 128, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 137, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 137, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 143, Col 5 - Metrics/BlockLength - Block has too many lines. [33/25]
❗ - Line 171, Col 25 - Style/EmptyElse - Redundant else-clause.
❗ - Line 174, Col 7 - Style/Next - Use next to skip iteration.
❗ - Line 52, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 52, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 56, Col 60 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 57, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 57, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 61, Col 60 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 62, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 62, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 71, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 71, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 80, Col 45 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 81, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 81, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 82, Col 1 - Style/EmptyLinesAroundMethodBody - Extra empty line detected at method body end.
❗ - Line 91, Col 47 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 91, Col 48 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❕ - Line 10, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for add_targeted_inventory_collections is too high. [43.12/20]
❕ - Line 140, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for parse_ems_events! is too high. [26.31/20]
❕ - Line 140, Col 3 - Metrics/CyclomaticComplexity - Cyclomatic complexity for parse_ems_events! is too high. [13/6]
❕ - Line 140, Col 3 - Metrics/MethodLength - Method has too many lines. [36/25]
❕ - Line 140, Col 3 - Metrics/PerceivedComplexity - Perceived complexity for parse_ems_events! is too high. [9/7]
❕ - Line 1, Col 1 - Metrics/ClassLength - Class has too many lines. [139/100]
❕ - Line 1, Col 121 - Metrics/LineLength - Line is too long. [123/120]

durandom · 2017-01-20T12:56:05Z

While I think it's great to see Inventory updates by the payload of an event, I'm still not sure if using the normal refresh code path is the right direction. As mentioned here in the corresponding core PR.

I think there are basically two scenarios:

Use eventpayload to update inventory with the stuff in the payload
Use eventpayload to augment / support an update which requests more information from the provider

While 2. might re-use some of the refresh part, 1. probably will not.

I'd rather see something like Event -> Automate -> InventoryUpdate

Also this should be designed carefully as it will be the blueprint (as in copy - paste) for other providers.

@Ladas @blomquisg @agrare wdyt?

Ladas · 2017-01-23T09:57:50Z

I can't do Event -> Automate -> InventoryUpdate, since I want to batch events. The Event -> Automate is there now for determining ** this events should be involved in refresh, send it there as a target**

With this way, we add a lot of targets to the refresh queue, lets say there can be 1000EmsEvent, 500Vm, 2Ems. Now in refresher we say, it's time for a refresh and it needs to decide what to do with all those targets.

So now based on configurations, we should have a new factories, that are able to process targets and output InventoryCollections and Data that will be used in a refresh. The configurations can be.

Always do a full ems refresh
Use data from the events to do a refresh without API calls
Use the data from a refresh to get ems_refs, based on those we will do batched targeted refreshes (using API calls to get a subset of the entities, mentioned in the events)
Combination of ^?

And ultimately, all the EmsEvents should be sent as an 'update_only node' to each graph refresh, so all the references are filled. (remember that we need a post refresh hook now, to tie an EmsEvent to a created entity, since the entity exists in our DB after the refresh and parser tries to tie them before the refresh. We are missing this in most of the providers.)

So what I think we should have, is a new class (Refresher::TargetProcessor) that is able to look at what was sent for refresh and it will determine what should be refresh based on configuration. So the output should be what InventoryCollections we will be saving (when we create IC, we also say how we will save it). And how do we fill those ICs with data (I am experimenting with 1 parser code, that have changed input data collections)

@durandom @blomquisg @agrare wdyt?

Ladas · 2017-01-27T17:12:57Z

Closing in favor of #115

The refresh based on event's payload can be used in another cloud providers, but it doesn't work well for AWS.

gberginc reviewed Jan 16, 2017

View reviewed changes

gberginc mentioned this pull request Jan 16, 2017

Introduce Amazon Block Storage Manager (EBS) #101

Merged

aiperon reviewed Jan 17, 2017

View reviewed changes

miq-bot added the unmergeable label Jan 17, 2017

Ladas added enhancement euwe/no labels Jan 17, 2017

Ladas force-pushed the ems_event_collection_refresh branch from f5fb4e7 to d5fc6bc Compare January 17, 2017 13:06

miq-bot removed the unmergeable label Jan 17, 2017

miq-bot added the unmergeable label Jan 18, 2017

Ladas force-pushed the ems_event_collection_refresh branch from d5fc6bc to b2baa02 Compare January 20, 2017 09:29

miq-bot removed the unmergeable label Jan 20, 2017

Ladas added 9 commits January 20, 2017 12:38

Remove single event payload refresh

2fff138

Remove single event payload refresh

Add EmsEventCollection target

4580c11

Add EmsEventCollection target allowing us to refresh multiple avents at once

Add ems_event_collection collection wrapper

17865c7

Add ems_event_collection collection wrapper allowing us to group EmsEvents and collect all related events.

Add attr_readers to inventory for each source of data

3bf18a0

Add attr_readers to inventory for each source of data

Group all ems events and parse them all

2d8ed9e

Group all ems events into EmsEventCollection and parse then regardless if they come from Cloud or Network Manager.

Add EmsEventCollection target to factory

0dcfd49

Add EmsEventCollection target to factory

Enhance Inventory::Targets base class

8768acb

Enhance Inventory::Targets base class

Move the array filtering to parser side

463b461

Move the array filtering to parser side, inventory side should have only API filtering

Fix rubocop issues

9c9273b

Fix rubocop issues

Ladas force-pushed the ems_event_collection_refresh branch from b2baa02 to 9c9273b Compare January 20, 2017 11:38

blomquisg requested a review from Fryguy January 25, 2017 14:52

Ladas closed this Jan 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ems event collection refresh #105

Ems event collection refresh #105

Ladas commented Jan 13, 2017 •

edited

Loading

gberginc left a comment

gberginc Jan 16, 2017

Ladas Jan 17, 2017

gberginc Jan 16, 2017 •

edited by chessbyte

Loading

Ladas Jan 17, 2017

Ladas Jan 20, 2017

gberginc Jan 16, 2017

Ladas Jan 17, 2017

gberginc Jan 16, 2017

Ladas Jan 17, 2017

gberginc Jan 16, 2017

Ladas Jan 17, 2017

aiperon Jan 17, 2017

gberginc Jan 17, 2017

Ladas Jan 17, 2017

Ladas commented Jan 17, 2017

gberginc commented Jan 17, 2017

Ladas commented Jan 17, 2017

miq-bot commented Jan 17, 2017

miq-bot commented Jan 18, 2017

durandom commented Jan 20, 2017

miq-bot commented Jan 20, 2017

durandom commented Jan 20, 2017

Ladas commented Jan 23, 2017 •

edited

Loading

Ladas commented Jan 27, 2017

Ems event collection refresh #105

Ems event collection refresh #105

Conversation

Ladas commented Jan 13, 2017 • edited Loading

gberginc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gberginc Jan 16, 2017 • edited by chessbyte Loading

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

Ladas commented Jan 17, 2017

gberginc commented Jan 17, 2017

Ladas commented Jan 17, 2017

miq-bot commented Jan 17, 2017

miq-bot commented Jan 18, 2017

durandom commented Jan 20, 2017

miq-bot commented Jan 20, 2017

durandom commented Jan 20, 2017

Ladas commented Jan 23, 2017 • edited Loading

Ladas commented Jan 27, 2017

Ladas commented Jan 13, 2017 •

edited

Loading

gberginc Jan 16, 2017 •

edited by chessbyte

Loading

Ladas commented Jan 23, 2017 •

edited

Loading