Azure: Event support #7439

bronaghs · 2016-03-21T17:02:07Z

Event support for Azure.

A couple of points to note:

There is no payload included in the Azure event making it difficult to gather details.
The Azure "write" event is fired after any configuration change has been made to a VM e.g. a change of flavor assignment, the addition of a new data disk etc, but it also is fired upon the creation of a new VM. Given the lack of payload and absence of any description, it is impossible to determine which scenario triggered the event.

Corresponding Trello card:
https://trello.com/c/QnXCQPZj/50-3-azure-events-event-mappings

durandom · 2016-03-22T14:50:28Z

app/models/manageiq/providers/azure/cloud_manager/event_catcher/stream.rb

+    Azure::Armrest::Insights::EventService.new(conf)
+  end
+
+  def each


I think each is not used anywhere. I removed it from the aws eventstream :)

chessbyte · 2016-03-22T15:12:05Z

@blomquisg @Fryguy please review

bronaghs · 2016-03-30T19:08:39Z

@blomquisg @Fryguy - ready for review

durandom · 2016-03-31T08:39:55Z

app/models/manageiq/providers/azure/cloud_manager/event_catcher/stream.rb

+    while @collecting_events
+
+      # Grab only events for the last minute if this is the first poll
+      startup_interval = (Time.current - 1.minute).httpdate


this is only used for the first iteration of the loop.
how about inlining it or extracting a method?

you might even consider extracting the whole filter logic into their own methods and leave each_batch only concerned with yielding events

@durandom - Yea, doing this made the code that bit more readable I think.

blomquisg · 2016-04-04T21:43:47Z

app/models/manageiq/providers/azure/cloud_manager/event_catcher/stream.rb

+    Time.zone.parse(events.last.event_timestamp).to_f + 0.001
+  end
+
+  def print_timestamp(time)


nitpick: can you rename this method to format_timestamp? When I read it, I thought it was going to output the timestamp somewhere. :)

blomquisg · 2016-04-04T22:00:12Z

A couple nitpicks. Otherwise, LGTM

@Fryguy can you look at the config updates?

@gmcculloug can you look at the event handling updates?

gmcculloug · 2016-04-05T12:30:50Z

@lfu Please review the event handling and suggest policy event actions where needed.

Fryguy · 2016-04-06T14:37:54Z

app/models/manageiq/providers/azure/cloud_manager/event_catcher/runner.rb

+    # We do not care for 'Begin' events, only the 'End' events which indicate
+    # the action the event relates to was completed successfully.
+    event_type = parse_event_type(event)
+    filtered_events.include?(event_type) || event_type.end_with?("BeginRequest")


I'm not sure why we wouldn't collect Begin events...in other providers we try to collect everything.

Fryguy · 2016-04-06T14:49:16Z

...grate/data/20160127210624_convert_configurations_to_settings_changes/event_handling.tmpl.yml

@@ -130,6 +130,7 @@ event_groups:
    - orchestration.autoscaling.error
    - servergroup.update
    - servergroup.addmemeber
+    - virtualMachines_write_EndRequest


These changes should not be in this file...they should be in settings.yml

Fryguy · 2016-04-06T14:57:06Z

@bronaghs Looks really good! Most of my comments were style and structure, so relatively minor.

bronaghs · 2016-04-06T14:59:47Z

thanks @Fryguy !

miq-bot · 2016-04-11T15:08:05Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

bronaghs · 2016-04-11T15:13:13Z

@blomquisg @Fryguy @durandom @chessbyte
I addressed your feedback and kept the changes in a separate commit to make it easy to identify. I don't think i rejected any of your feedback, it was all helpful. Thanks for taking the time to review it.

lfu · 2016-04-11T16:06:03Z

The event switchboard modeling looks good.

miq-bot · 2016-04-11T16:07:31Z

Checked commits bronaghs/manageiq@5f81bc2~...bdfff05 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 2 offenses detected

app/models/manageiq/providers/azure/cloud_manager/event_catcher/stream.rb

🔶 - Line 31, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.

spec/models/manageiq/providers/azure/cloud_manager/event_catcher/runner_spec.rb

🔹 - Line 13, Col 121 - Metrics/LineLength - Line is too long. [126/120]

durandom · 2016-04-12T15:27:05Z

looks good to me, nice job @bronaghs 👍

only as a side note for future specs, if you run this...

❯ be rspec --format documentation spec/models/manageiq/providers/azure/cloud_manager/event_catcher/runner_spec.rb

ManageIQ::Providers::Azure::CloudManager::EventCatcher::Runner
  parsing properties
    parse properties
      event type
      vm ref

...it should read like documentation. That also helps reading the specs in order to get a clue on how the code is supposed to work and to fix broken specs. It also helps structuring the specs in a nice clean way.

blomquisg · 2016-04-12T16:52:45Z

What the heck @durandom?! Holding out on the cool tricks! :)

bronaghs force-pushed the azure_events branch 3 times, most recently from e0e3dd4 to 2ecf321 Compare March 21, 2016 17:53

chessbyte added enhancement providers/azure labels Mar 21, 2016

durandom reviewed Mar 22, 2016
View reviewed changes

bronaghs changed the title ~~Azure: Event catcher support~~ Azure: Event support Mar 22, 2016

bronaghs force-pushed the azure_events branch from 124f029 to 54fabe8 Compare March 30, 2016 17:55

miq-bot added the dependencies label Mar 30, 2016

chessbyte closed this Mar 30, 2016

chessbyte reopened this Mar 30, 2016

durandom reviewed Mar 31, 2016
View reviewed changes

bronaghs force-pushed the azure_events branch from 54fabe8 to 8949a49 Compare April 1, 2016 17:25

blomquisg reviewed Apr 4, 2016
View reviewed changes

gmcculloug added the core/events label Apr 5, 2016

Fryguy reviewed Apr 6, 2016
View reviewed changes

bronaghs force-pushed the azure_events branch from 8949a49 to 536a41c Compare April 11, 2016 15:05

Bronagh Sorota added 2 commits April 11, 2016 11:09

Azure event catcher support

5f81bc2

Addresses Azure events support PR review feedback

bdfff05

bronaghs force-pushed the azure_events branch from 536a41c to bdfff05 Compare April 11, 2016 15:10

blomquisg merged commit 1b97821 into ManageIQ:master Apr 12, 2016

blomquisg added this to the Sprint 39 Ending Apr 18, 2016 milestone Apr 12, 2016

durandom mentioned this pull request Apr 18, 2016

Add event catcher support for Google provider #8003

Merged

Fryguy added the providers/cloud label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure: Event support #7439

Azure: Event support #7439

bronaghs commented Mar 21, 2016

durandom Mar 22, 2016

chessbyte commented Mar 22, 2016

bronaghs commented Mar 30, 2016

durandom Mar 31, 2016

bronaghs Apr 1, 2016

blomquisg Apr 4, 2016

blomquisg commented Apr 4, 2016

gmcculloug commented Apr 5, 2016

Fryguy Apr 6, 2016

Fryguy Apr 6, 2016

Fryguy commented Apr 6, 2016

bronaghs commented Apr 6, 2016

miq-bot commented Apr 11, 2016

bronaghs commented Apr 11, 2016

lfu commented Apr 11, 2016

miq-bot commented Apr 11, 2016

durandom commented Apr 12, 2016

blomquisg commented Apr 12, 2016

Azure: Event support #7439

Azure: Event support #7439

Conversation

bronaghs commented Mar 21, 2016

Event support for Azure.

A couple of points to note:

durandom Mar 22, 2016

Choose a reason for hiding this comment

chessbyte commented Mar 22, 2016

bronaghs commented Mar 30, 2016

durandom Mar 31, 2016

Choose a reason for hiding this comment

bronaghs Apr 1, 2016

Choose a reason for hiding this comment

blomquisg Apr 4, 2016

Choose a reason for hiding this comment

blomquisg commented Apr 4, 2016

gmcculloug commented Apr 5, 2016

Fryguy Apr 6, 2016

Choose a reason for hiding this comment

Fryguy Apr 6, 2016

Choose a reason for hiding this comment

Fryguy commented Apr 6, 2016

bronaghs commented Apr 6, 2016

miq-bot commented Apr 11, 2016

bronaghs commented Apr 11, 2016

lfu commented Apr 11, 2016

miq-bot commented Apr 11, 2016

durandom commented Apr 12, 2016

blomquisg commented Apr 12, 2016