Improve Targeted Refresh for Cloud and Network managers #175

mansam · 2017-12-07T20:59:08Z

https://bugzilla.redhat.com/show_bug.cgi?id=1519214

I'm basing the events I catch on ceilometer event definitions, but I'm not sure that I have a complete list. @aufi, do you know if there is a better source than https://github.com/openstack/ceilometer/blob/master/ceilometer/pipeline/data/event_definitions.yaml ?

Ladas · 2017-12-08T08:42:31Z

app/models/manageiq/providers/openstack/network_manager/event_target_parser.rb

+  # @param ems_event [EmsEvent] EmsEvent object
+  # @return [Array] Array of ManagerRefresh::Target objects
+  def parse_ems_event_targets(ems_event)
+    target_collection = ManagerRefresh::TargetCollection.new(:manager => ems_event.ext_management_system, :event => ems_event)


Hm, the separate event catcher for network manager complicates stuff.

I think we need to set here parent_manager:
ManagerRefresh::TargetCollection.new(:manager => ems_event.ext_management_system.parent_manager, :event => ems_event)

Since all of the targeted refresh runs in the cloud manager and the targets are bucketed based on manager. Storage wise, this doesn't matter, since each persistor assigns the right manager for saving. So this is more for which ems and therefore worker should process it.

Ladas · 2017-12-08T08:47:42Z

@mansam so for /System/event_handlers/refresh in automate to work (to parse some targets and send them for refresh), we need to cover all events listed in https://github.com/Ladas/manageiq-content/tree/33e254dd0fcc2e47f379130f8dcf21b51ab30e10/content/automate/ManageIQ/System/Event/EmsEvent/OpenStack.class

But if we could base it on generic definition like https://github.com/openstack/ceilometer/blob/master/ceilometer/pipeline/data/event_definitions.yaml, that would be awesome. That would mean it will work for more events than defined in our automate.

mansam · 2017-12-08T20:19:59Z

@Ladas @aufi The list of events from manageiq-content is very helpful, but unfortunately I'm not sure it's sufficient. If I understand correctly, we capture events from Ceilometer, and Ceilometer changes the format of the events from the actual services quite a bit. Without more detail about the actual event definitions we get from Ceilometer it's not really possible to parse the events (since I don't know what fields are available, and they are not consistently named), and not every event in manageiq-content has an equivalent in the default Ceilometer event definition yaml. I've done a lot of looking over the past couple days and in the past when first working on this and am coming up empty handed for better documentation.

(as a concrete example-- what are the fields on the aggregate.*.* events after they pass through Ceilometer?)

Ladas · 2017-12-08T20:53:09Z

@mansam right so, AWS has no docs for the event payloads, so what I ended up doing was

+++ b/app/models/manageiq/providers/amazon/cloud_manager/event_catcher/stream.rb
@@ -190,6 +190,13 @@ class ManageIQ::Providers::Amazon::CloudManager::EventCatcher::Stream
       return
     end
 
+    ####
+    File.open(Rails.root.join("events", event["eventType"]+".json" ), "w") do |file|
+      file.write(message.body)
+    end
+    ####
+
+
     $log.info("#{log_header} Found SNS Message with message type #{event["eventType"]} coming from #{event[:event_source]}")

Then I was clicking around, recording the real events. Then I am using the payload of the real events for the specs, to make sure I am parsing the right targets. Also, for CloudWatch events, the ids are in a form of "instance_id, image_id", so I don't even care what kind of event it is. Any event referencing instance_id, will produce VM target.

So yeah, if the event payloads are different in AMQP/Ceilometer/WhatIsTheNewServiceName, than this kind of sucks.

The AWS has similar issue, sometimes the event has VpcID, other times vpcId https://github.com/Ladas/manageiq-providers-amazon/blob/4c66893fd1a91dc5fd3aa259a7c0ddd044c65576/app/models/manageiq/providers/amazon/cloud_manager/event_target_parser.rb#L96

mansam · 2017-12-09T00:19:04Z

@Ladas Hrm, good point that if the IDs are named in a recognizable fashion then it doesn't matter at all what event it came from.

miq-bot · 2017-12-13T23:59:20Z

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

miq-bot · 2018-01-05T15:38:06Z

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

blomquisg · 2018-01-05T17:32:33Z

@mansam, can you rebase and repush?

miq-bot · 2018-01-05T19:03:50Z

Some comments on commits mansam/manageiq-providers-openstack@6a91a7e~...9b4dbfd

spec/models/manageiq/providers/openstack/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.

spec/models/manageiq/providers/openstack/network_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 · 2018-01-05T19:03:51Z

Checked commits mansam/manageiq-providers-openstack@6a91a7e~...9b4dbfd with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
37 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

mansam · 2018-01-05T19:43:41Z

@blomquisg updated

blomquisg · 2018-01-09T19:10:46Z

@Ladas, do you still have outstanding changes you're looking for? If not, can you clear/change your review?

Ladas · 2018-01-09T19:18:53Z

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

-      ManageIQ::Providers::Openstack::CloudManager::RefreshParser.ems_inv_to_hashes(ems, refresher_options)
-    end
-
+  class Openstack::CloudManager::Refresher < ManageIQ::Providers::BaseManager::ManagerRefresher
    def save_inventory(ems, target, inventory_collections)
      super
      EmsRefresh.queue_refresh(ems.network_manager) if target.kind_of?(ManageIQ::Providers::BaseManager)
      EmsRefresh.queue_refresh(ems.cinder_manager) if target.kind_of?(ManageIQ::Providers::BaseManager)


so maybe, we will have to do a cinder full refresh always here, we should track that as a separate issue though

Ladas · 2018-01-09T19:35:06Z

app/models/manageiq/providers/openstack/refresh_parser_common/helper_methods.rb

@@ -3,15 +3,15 @@ module Openstack
    module RefreshParserCommon
      module HelperMethods
        def openstack_admin?
-          ::Settings.ems.ems_refresh.openstack.try(:is_admin)
+          ::Settings.ems_refresh.openstack.try(:is_admin)


a nit for other PR probably:

we usually abstract ::Settings.ems_refresh.openstack as a method , in new refresh parser it available on collector https://github.com/Ladas/manageiq/blob/b9e6b294604d744f59e3bf270e16264f067fb85a/app/models/manager_refresh/inventory/collector.rb#L11

I agree that it should be abstracted out in the future.

Ladas · 2018-01-09T19:38:13Z

spec/models/manageiq/providers/openstack/cloud_manager/refresher_rhos_juno_spec.rb

@@ -49,16 +49,16 @@

        assert_common
      end
-      ::Settings.ems.ems_refresh.openstack.is_admin = false
-      ::Settings.ems.ems_refresh.openstack_network.is_admin = false
+      ::Settings.ems_refresh.openstack.is_admin = false


as commented in other PR, this should use

https://github.com/Ladas/manageiq-providers-amazon/blob/7b1a7226dea0566fd17d3b06271782e213bf4969/spec/models/manageiq/providers/amazon/aws_refresher_spec_common.rb#L47

Best to clean them all up in a refactoring PR

Ladas

Looks good overall. 👍

@mansam I have commented several nits though, that should be fixed in other PRs.

Also for other PRs, seems like we are mostly doing API query for 1 target, so we should talk about how to optimize that and probably set upper limit of targets for now, before we would switch to full refresh. (we do not need it for AWS since there is 1 API for N targets)

Also, it's good to read the targeted VCRs to verify it collects what it should. In targeted_vm I see we collect the same port several times. Also we fetch all subnets,
so API filtering probably doesn't work. (these are minor perf issues though)

mansam · 2018-01-09T19:56:27Z

@Ladas Thanks for the review. In the future for APIs that support filtering correctly, it might make sense to make a list request that is filtered by the desired IDs. Unfortunately filtering doesn't seem to be supported everywhere (like networking, it seems).

aufi · 2018-01-10T09:06:40Z

LGTM, merging!

Improve Targeted Refresh for Cloud and Network managers (cherry picked from commit 92aed61) https://bugzilla.redhat.com/show_bug.cgi?id=1533225

simaishi · 2018-01-10T19:10:14Z

Gaprindashvili backport details:

$ git log -1
commit 1c1ba1a8be210bbcfd109b777cedb89af0e507e7
Author: Marek Aufart <aufi.cz@gmail.com>
Date:   Wed Jan 10 10:06:54 2018 +0100

    Merge pull request #175 from mansam/event-target-parser-network-manager
    
    Improve Targeted Refresh for Cloud and Network managers
    (cherry picked from commit 92aed61171736d5d96d11bbf25a609347d29727b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1533225

miq-bot added the wip label Dec 7, 2017

Ladas suggested changes Dec 8, 2017

View reviewed changes

mansam force-pushed the event-target-parser-network-manager branch from a202ee8 to e016c56 Compare December 8, 2017 20:49

mansam force-pushed the event-target-parser-network-manager branch 2 times, most recently from 5875a5e to 4209c87 Compare December 11, 2017 21:09

mansam changed the title ~~[wip] Implement EventTargetParser for NetworkManager~~ Implement EventTargetParser for NetworkManager Dec 13, 2017

miq-bot removed the wip label Dec 13, 2017

miq-bot added the unmergeable label Dec 13, 2017

mansam force-pushed the event-target-parser-network-manager branch from b401bf4 to 6ba0a3b Compare December 14, 2017 00:01

mansam changed the title ~~Implement EventTargetParser for NetworkManager~~ [wip] Implement EventTargetParser for NetworkManager Dec 14, 2017

mansam changed the title ~~[wip] Implement EventTargetParser for NetworkManager~~ [wip] Improve Targeted Refresh for Cloud and Network managers Dec 14, 2017

miq-bot added wip and removed unmergeable labels Dec 14, 2017

mansam force-pushed the event-target-parser-network-manager branch 2 times, most recently from fc910da to 4437dd7 Compare December 20, 2017 21:44

mansam changed the title ~~[wip] Improve Targeted Refresh for Cloud and Network managers~~ Improve Targeted Refresh for Cloud and Network managers Dec 21, 2017

mansam mentioned this pull request Dec 21, 2017

Move Openstack refresh settings under the root where they belong ManageIQ/manageiq-schema#146

Merged

miq-bot removed the wip label Dec 21, 2017

miq-bot added the unmergeable label Jan 5, 2018

chessbyte assigned aufi Jan 5, 2018

chessbyte added blocker bug labels Jan 5, 2018

Implement EventTargetParser for NetworkManager

6a91a7e

mansam added 4 commits January 5, 2018 13:35

fix lint

f10d3cf

Fix openstack settings location

d2fdf9f

Refactor refreshers to inherit from BaseManager::ManagerRefresher

9bdd5c1

Update VCRs

9b4dbfd

mansam force-pushed the event-target-parser-network-manager branch from 4437dd7 to 9b4dbfd Compare January 5, 2018 19:01

miq-bot removed the unmergeable label Jan 5, 2018

Ladas reviewed Jan 9, 2018

View reviewed changes

Ladas approved these changes Jan 9, 2018

View reviewed changes

aufi merged commit 92aed61 into ManageIQ:master Jan 10, 2018

aufi added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 10, 2018

aufi added the gaprindashvili/yes label Jan 10, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 10, 2018

aufi mentioned this pull request Jan 11, 2018

Targeted Refresh is not triggered ManageIQ/manageiq#16759

Closed

AlonaKaplan mentioned this pull request Jan 21, 2018

Move refresh settings for the Network Manager ManageIQ/manageiq-providers-ovirt#198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Targeted Refresh for Cloud and Network managers #175

Improve Targeted Refresh for Cloud and Network managers #175

mansam commented Dec 7, 2017 •

edited

Loading

Ladas Dec 8, 2017

Ladas commented Dec 8, 2017

mansam commented Dec 8, 2017 •

edited

Loading

Ladas commented Dec 8, 2017 •

edited

Loading

mansam commented Dec 9, 2017

miq-bot commented Dec 13, 2017

miq-bot commented Jan 5, 2018

blomquisg commented Jan 5, 2018

miq-bot commented Jan 5, 2018

miq-bot commented Jan 5, 2018

mansam commented Jan 5, 2018

blomquisg commented Jan 9, 2018

Ladas Jan 9, 2018

Ladas Jan 9, 2018

mansam Jan 9, 2018

Ladas Jan 9, 2018

Ladas left a comment

mansam commented Jan 9, 2018

aufi commented Jan 10, 2018

simaishi commented Jan 10, 2018

Improve Targeted Refresh for Cloud and Network managers #175

Improve Targeted Refresh for Cloud and Network managers #175

Conversation

mansam commented Dec 7, 2017 • edited Loading

Ladas Dec 8, 2017

Choose a reason for hiding this comment

Ladas commented Dec 8, 2017

mansam commented Dec 8, 2017 • edited Loading

Ladas commented Dec 8, 2017 • edited Loading

mansam commented Dec 9, 2017

miq-bot commented Dec 13, 2017

miq-bot commented Jan 5, 2018

blomquisg commented Jan 5, 2018

miq-bot commented Jan 5, 2018

miq-bot commented Jan 5, 2018

mansam commented Jan 5, 2018

blomquisg commented Jan 9, 2018

Ladas Jan 9, 2018

Choose a reason for hiding this comment

Ladas Jan 9, 2018

Choose a reason for hiding this comment

mansam Jan 9, 2018

Choose a reason for hiding this comment

Ladas Jan 9, 2018

Choose a reason for hiding this comment

Ladas left a comment

Choose a reason for hiding this comment

mansam commented Jan 9, 2018

aufi commented Jan 10, 2018

simaishi commented Jan 10, 2018

mansam commented Dec 7, 2017 •

edited

Loading

mansam commented Dec 8, 2017 •

edited

Loading

Ladas commented Dec 8, 2017 •

edited

Loading