Separate collector from inventory and targeted refresh #115
Conversation
|
This pull request is not mergeable. Please rebase and repush. |
Separate a Collector from an Inventory. A collector class takes care of data collection from a provider while inventory creates InventoryCollections and associates right Collector object
Remove EventPayloadVm target, we don't use it anymore.
Ladas
force-pushed
the
separate_collector_from_inventory
branch
from
0cdd04f
to
be66154
Compare
Ladas
changed the title
Ladas
force-pushed
the
separate_collector_from_inventory
branch
2 times, most recently
from
7c5c1b4
to
c19ca78
Compare
There was a problem hiding this comment.
I like the collector approach and having Inventory classes for every possible target.
But still, this PR does too many things. The most controversial part being the EmsEvent as a target and hence the need for a TargetCollection
Sorry, but I'd like to see that split into smaller PRs still.
Maybe
- modelling the Collector/Target/Inventory thing first in the core on a smaller ansible provider
- moving that modelling over here to aws
- finally think about the EmsEvent as a target
wdyt>
| @@ -26,6 +26,15 @@ def parse_targeted_inventory(ems, _target, inventory) | |||
| hashes, = Benchmark.realtime_block(:parse_inventory) do | |||
| if refresher_options.try(:[], :inventory_object_refresh) | |||
| ManageIQ::Providers::Amazon::CloudManager::RefreshParserInventoryObject.new(inventory).populate_inventory_collections | |||
There was a problem hiding this comment.
so, what does this do? I guess its a leftover?
There was a problem hiding this comment.
oh no, it's because this manipulates inventory
| @@ -26,6 +26,15 @@ def parse_targeted_inventory(ems, _target, inventory) | |||
| hashes, = Benchmark.realtime_block(:parse_inventory) do | |||
| if refresher_options.try(:[], :inventory_object_refresh) | |||
There was a problem hiding this comment.
probably you could even get rid of this. Because inventory is always nil for the old refresh
| if inventory.kind_of?(ManageIQ::Providers::Amazon::Inventory::Targets::TargetCollection) | ||
| # 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 |
There was a problem hiding this comment.
so, this feels somehow wrong.
Because you use RefreshParserInventoryObject to manipulate inventory right?
| @@ -35,6 +44,29 @@ def parse_targeted_inventory(ems, _target, inventory) | |||
| hashes | |||
| end | |||
|
|
|||
| def preprocess_targets | |||
There was a problem hiding this comment.
For now its ok, but I dont think we should allow this to be overriden
| _log.info "Defaulting to full refresh for EMS: [#{ems.name}], id: [#{ems.id}], from targets: #{targets_for_log}" if targets.length > 1 | ||
| end | ||
|
|
||
| # We want all targets of class EmsEvent to be merged into one target, so they can be refreshed together, otherwise |
There was a problem hiding this comment.
So, this thing about EmsEvent being a target. Is ManageIQ/manageiq#13408 needed for this to work?
| @@ -0,0 +1,126 @@ | |||
| class ManageIQ::Providers::Amazon::Inventory::Collectors | |||
There was a problem hiding this comment.
I dont like this class being a plural. It should be Collector
There was a problem hiding this comment.
we can do singular, I have there Targets and Collectors
| initialize_inventory_sources | ||
| end | ||
|
|
||
| def initialize_inventory_sources |
There was a problem hiding this comment.
So every collector has all this? Shouldnt it be per Subclass different?
There was a problem hiding this comment.
no, we need is always defined, so by default all data are []
| @@ -1,4 +1,9 @@ | |||
| class ManageIQ::Providers::Amazon::Inventory::Targets < ManageIQ::Providers::Amazon::Inventory | |||
There was a problem hiding this comment.
Same here, why is that a plural class name? And why does it inherit from Inventory ?
There was a problem hiding this comment.
The target is really an inventory here
We can revisit that Inventory could have target and collector
CloudManager target typo
CloudManager collector rubocop issue
All refs in base collector are a Set, so they check uniqueness by default. Also adding base entries for block storage models.
Ladas
force-pushed
the
separate_collector_from_inventory
branch
from
31149cd
to
443ef96
Compare
|
Checked commits Ladas/manageiq-providers-amazon@5533c2d~...443ef96 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/manageiq/providers/amazon/inventory/collectors.rb
app/models/manageiq/providers/amazon/inventory/collectors/cloud_manager.rb
app/models/manageiq/providers/amazon/inventory/collectors/network_manager.rb
app/models/manageiq/providers/amazon/inventory/targets.rb
app/models/manageiq/providers/amazon/inventory/targets/cloud_manager.rb
|
|
merging to move forward ontop of this |
Separate collector from inventory, Collector class focus directly on collecting related data from a provider. Introduce targeted refresh.