-
Notifications
You must be signed in to change notification settings - Fork 900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
collectors, targets and parsers #13728
Conversation
@@ -1,2 +1,3 @@ | |||
class ManageIQ::Providers::AnsibleTower::AutomationManager::RefreshWorker::Runner < ManageIQ::Providers::BaseManager::RefreshWorker::Runner | |||
# FIXME why do we even need this class? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's used in the Worker code depth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is the physical process that MIQ monitors for the AnsibleTower Automation Manager refresh worker. Whereas the AnsibleTower::AutomationManager::RefreshWorker
is the ApplicationRecord
that is stored in the miq_workers
table for capturing the details of this process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I know, this was more an inline rant - sorry
@@ -34,6 +34,8 @@ def initialize(model_class, manager_ref: nil, association: nil, parent: nil, str | |||
@internal_attributes = [:__feedback_edge_set_parent] | |||
@complete = complete.nil? ? true : complete | |||
@update_only = update_only.nil? ? false : update_only | |||
@builder_params = builder_params | |||
@inventory = inventory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inventory is not used?
attr_accessor :parser | ||
|
||
def initialize(root) | ||
@root = root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is root the ems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best to call it manager or ems, it will always be that?
log_header = format_ems_for_logging(ems) | ||
_log.debug "#{log_header} Parsing inventory..." | ||
hashes, = Benchmark.realtime_block(:parse_inventory) do | ||
ManageIQ::Providers::AnsibleTower::AutomationManager::RefreshWorker::Parser.new(ems, target, inventory).parse | ||
inv_target = ManageIQ::Providers::AnsibleTower::Inventory::Targets::AutomationManager.new(target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I think we should use the same builder across all refreshers, so we don't solve these details here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapping my head around this ... when the target is the manager, then it's a full refresh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, just another target
attr_accessor :collector | ||
|
||
def initialize(collector) | ||
@collector = collector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could pass Inventory object here, or we need to pass more args, collector, target, options, maybe ems.
we might not need ems, if we would pass the ems in builder_params. Also options could be accessed through collector
inv_target = ManageIQ::Providers::AnsibleTower::Inventory::Targets::AutomationManager.new(target) | ||
inv_target.parser = ManageIQ::Providers::AnsibleTower::Inventory::Parsers::AutomationManager.new(collector) | ||
inv_target.parse | ||
inv_target.inventory_collections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not crazy about this block ...
I don't think the inv_target
should contain the parser
, and I don't think the inv_target
should delegate to the parser
to parse
.
It's more straightforward to me to have:
# target = Active Record Target
# create inventory target
inv_target = ManageIQ::Providers::AnsibleTower::Inventory::Targets::AutomationManager.new(target)
# create inventory parser
parser = ManageIQ::Providers::AnsibleTower::Inventory::Parsers::AutomationManager.new(collector)
# parser parses target using given inventory collector
parser.parse(inv_target)
# parser returns parsed inventory collections
parser.inventory_collections
Or, even have the parse
method return the inventory_collections
?
But, I can't wrap my head about having the inv_target
knowing how to parse itself. Happy to talk about it, maybe I just need to see it from a different perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for me, best will be to pass both collector and target
..Parsers::AutomationManager.new(collector, target)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but still I'd like to have collector, target and parsers wrapped into the Inventory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is how Inventory looks for me
inventory = ManageIQ::Providers::Amazon::Inventory::new(
ems,
raw_target,
:collector_class => ManageIQ::Providers::Amazon::Inventory::Collector::NetworkManager,
:target_class => ManageIQ::Providers::Amazon::Inventory::Target::NetworkManager,
:parsers_classes => [ManageIQ::Providers::Amazon::NetworkManager::RefreshParserInventoryObject]
)
then you call just inventory.parse
I don't have the parser moved
@agrare ^ you should be able to combine any collector, target and parsers together to create a consistent Inventory refresh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @blomquisg , exactly the kind of feedback I was looking for. Make it easy to follow for people.
@Ladas I think that block should be done in the Inventory, right.
In general, I like this idea. But, let's talk about my comment. |
@durandom do you need a parser per target type? I'd think there would be a lot of overlap assuming you get mostly the same inventory for the different targets. If your targets are a Host and a VM, the inventory collection process will be different but once you have the inventory collection I think you should be able to use the same parser for the different inventory classes, thoughts? |
Hm I might have misunderstood this
But it looks later on like you have a parser per manager type not target type which would make more sense, if so scratch my previous comment :) |
targets_to_collectors.map do |target, collector_class| | ||
_log.info "#{log_header} New Inventory Collector for #{target.class} [#{target.try(:name)}] id: [#{target.id}]..." | ||
collector = collector_class.new(ems, target) | ||
_log.info "#{log_header} New Inventory Collector for...Complete" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ...Complete
log line and the log line from two lines above should both contain provider details. It makes tracking refreshes in the log easier when dealing with multiple providers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it does provide details with log_header
which is "EMS: [#{ems.name}], id: [#{ems.id}]"
Isnt that enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be enough information about the provider but if you're printing the target class/name/id in the opening line it should be exactly the same in the following line just with ...Complete
at the end of it so it is easy to correlate the start&end times in the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in cases where we want to collect all inventory up front (selfishly thinking of VMware 😄) I'd image we would do that here and collect the inventory as part of the initialize
method of the Inventory::Collector
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd image we would do that here and collect the inventory as part of the initialize method of the Inventory::Collector class
Yes, exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agrare thanks for the explanation of the log, understood. I'll add that
[target, collector] | ||
end | ||
else | ||
collect_legacy_inventory_for_targets(ems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I think you should just inline this method now. Pull the comment up from the method body for now, until we can completely do away with the "legacy" concept.
You might have to clean up some provider impls, but I think that's better than holding onto this poorly named method. Who named this anyway? 🐑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, looks like collect_legacy_inventory_for_targets
isnt used anywhere anymore in any provider. So lets remove it, yeah
|
||
delegate :each, :size, :to => :to_a | ||
|
||
def initialize(model_class, manager_ref: nil, association: nil, parent: nil, strategy: nil, saved: nil, | ||
custom_save_block: nil, delete_method: nil, data_index: nil, data: nil, dependency_attributes: nil, | ||
attributes_blacklist: nil, attributes_whitelist: nil, complete: nil, update_only: nil, | ||
check_changed: nil, custom_manager_uuid: nil, custom_db_finder: nil, arel: nil) | ||
attributes_blacklist: nil, attributes_whitelist: nil, complete: nil, update_only: nil, inventory: nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still unused :inventory argument?
_log.debug "#{log_header} Parsing inventory...Complete" | ||
inventory_collections | ||
else | ||
hashes, = Benchmark.realtime_block(:parse_legacy_inventory) { parse_legacy_inventory(ems) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right ... this is where I was really thinking should be inlined. But, this is truly delegated out to the providers for the legacy inventory collection. I guess it can stay for now, but I'm looking forward to this dying off soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, a lot are using it still though
app/models/manageiq/providers/foreman/configuration_manager/refresher.rb:6: def parse_legacy_inventory(manager)
app/models/manageiq/providers/foreman/provisioning_manager/refresher.rb:6: def parse_legacy_inventory(manager)
app/models/manageiq/providers/google/cloud_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/google/network_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/hawkular/datawarehouse_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/hawkular/middleware_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/kubernetes/container_manager/refresher.rb:6: def parse_legacy_inventory(ems)
app/models/manageiq/providers/microsoft/infra_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/nuage/network_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/openshift/container_manager/refresher.rb:27: def parse_legacy_inventory(ems)
app/models/manageiq/providers/openstack/cloud_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/openstack/infra_manager/refresher.rb:6: def parse_legacy_inventory(ems)
app/models/manageiq/providers/openstack/network_manager/refresher.rb:5: def parse_legacy_inventory(ems)
app/models/manageiq/providers/storage_manager/cinder_manager/refresher.rb:6: def parse_legacy_inventory(ems)
app/models/manageiq/providers/storage_manager/swift_manager/refresher.rb:5: def parse_legacy_inventory(ems)
plugins/manageiq-providers-azure/app/models/manageiq/providers/azure/cloud_manager/refresher.rb:5: def parse_legacy_inventory(ems)
plugins/manageiq-providers-azure/app/models/manageiq/providers/azure/network_manager/refresher.rb:5: def parse_legacy_inventory(ems)
plugins/manageiq-providers-lenovo/app/models/manageiq/providers/lenovo/physical_infra_manager/refresher.rb:5: def parse_legacy_inventory(ems)
plugins/manageiq-providers-vmware/app/models/manageiq/providers/vmware/cloud_manager/refresher.rb:4: def parse_legacy_inventory(ems)
plugins/manageiq-providers-vmware/app/models/manageiq/providers/vmware/network_manager/refresher.rb:4: def parse_legacy_inventory(ems)
end | ||
|
||
def job_templates | ||
manager.with_provider_connection do |connection| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we know we're going to call all of these methods in succession (e.g.: here) could we keep the connection open so we don't have to open/close it every time? I'm thinking mainly about rhev where it has to connect to a remote system without a connection broker the cost to open/close the sessions can add up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, in aws I have methods holding the connection cached https://github.com/Ladas/manageiq-providers-amazon/blob/a32f6e82199d72c731e413c89d110e0afacbda82/app/models/manageiq/providers/amazon/inventory/collector.rb#L88-L88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, and it was like this before.
is this a stable API? Is with_provider_connection
avail in every manager? Maybe we can hide this from the user somehow...
But falling back to keeping the connection for now
@@ -146,6 +149,11 @@ def object_index_with_keys(keys, object) | |||
keys.map { |attribute| object.public_send(attribute).to_s }.join(stringify_joiner) | |||
end | |||
|
|||
def find_or_build(manager_uuid) | |||
# FIXME: splat manager_ref | |||
data_index[manager_uuid] || build({ manager_ref.first => manager_uuid }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, manager_ref.first => manager_uuid is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to pass the uuid as a string, then I do
manager_uuids = manager_uuid.split(stringify_joiner)
hash_uuid_by_ref = manager_ref.zip(manager_uuids).to_h
then if manager ref is [:hardware, :device_name], the uuid in string is "#{hardware_ref}__#{device_name}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the best would be to do find_or_build_by(:hardware => xy, :device_name => xx) or find_or_build_by(:ems_ref => xy) ?
with a check that only allowed combination is manager_ref. I will be bringing ability to create multiple indexes on the InventoryCollection later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then the same would be for lazy_find_by
we have few places now, where I could use separate index, but then it can return multiple results, there is only one unique index, which is manager_ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the exception could nicely say, allowed find_by values are {:ems_ref => xy}, {:hardware => xy, :device_name => xx}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's keep it that way and do this in a follow up, where all the other manager_uuid
methods in here are also updated.
|
||
targets_to_collectors = targets.each_with_object({}) do |target, memo| | ||
# expect collector at <provider>/Inventory/Collectors/<target_name> | ||
memo[target] = "#{provider_module}::Inventory::Collectors::#{target.class.name.demodulize}".safe_constantize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how this will work for
ManageIQ::Providers::Amazon::Inventory::Collector::StorageManager::Ebs
@@ -0,0 +1,35 @@ | |||
class ManageIQ::Providers::AnsibleTower::Inventory::Parsers::AutomationManager < ManagerRefresh::Inventory::Parser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsers or Parser? Will we keep it all singular?
@@ -0,0 +1,33 @@ | |||
class ManageIQ::Providers::AnsibleTower::Inventory::Targets::AutomationManager < ManagerRefresh::Inventory::Target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ladas are you saying this should be rather
ManageIQ::Providers::AnsibleTower::Inventory::Target::AutomationManager < ManagerRefresh::Inventory::Target
all singular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I changed it to that, since you wanted singular names in my PR
so both dir and class are named Target, Parser, Collector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
@Ladas @blomquisg @agrare thanks for the feedback. I addressed most of it. And given the detail you focused on, I guess you are ok with the overall layout / structure / concept of this? If so, I'd squash the commits and we could build upon this. |
looks great overall 👍 , I will solve the AWS issues when is switch to using these base classes |
So @Fryguy said in gitter
Yes, thats what I'm thinking too. But then while you are requesting to refresh that target, you are also building up inventory for that target to hand it over to save_inventory to persist it. So, I'm not saying All in all I think it good enough, for now. And I'd opt for moving forward from that; see how it works out with different teams implementing a targeted refresh and doing adjustments further down the road. |
@blomquisg @agrare are you 👍 with this? |
So since this is at the same level as the manager how about the scenario where you have:
And that all works, but what about for the targets:
How would we target a cloud vm and an infra vm? |
Yeah, that'll break now. parser_class = "#{provider_module}::Inventory::Parser::#{target.class.name.demodulize}".safe_constantize How about addressing that later? |
61018b2
to
2354061
Compare
ok, squashed commits... |
@durandom @agrare for AWS, I have Inventory Builder, to create Inventory object with the right <collector, target, parser> based on the input target. So for more complex providers, I think we might go this way. It has a repeating code that can simplified. Then the base class could just check if the builder exists, otherwise it would build it automatically as @durandom has it now. |
👍 |
I'm 👍 with this, wish we could find a better name for |
Kicking the tests |
To simplify targeted refresh, we don't differentiate between targeted refresh and full refresh anymore, but a full refresh is simply the manager as a target. So in this ansible example, it's automation_manager. Collaborators Inventory/Collector A collector acts as a proxy to the providers inventory. It's task is to provide methods that collect this data. It's base class is initialized with manager and target, so it can make use of with_provider_connection or restrict collected data to the target scope. The target in this case is the ActiveRecord instance of the refresh we are targeting. So in a full refresh it's the same as the manager. If we target a Host, the target is the Host instance under the manager Inventory/Target This holds all InventoryCollections and makes them accessible via simple methods. Thats needed for cross referencing InventoryObjects via lazy_find Inventory/Parser The base class holds the above collector and inventory/target. The entry method is parse which fills the inventory collections by making use of the collector Inventory ManagerRefresh::Inventory takes care of wiring up the above 3 objects: Collector, Target and Parser. It's constructed in the platform and the instances of the 3 objects are constructed in the platform as well. It's all happening in ems_refresher_mixin mainly in parse_targeted_inventory. So ideally this is hidden from the provider author. What's required from a provider author? Basically the author provides these 3 classes, which inherit from their base classes in the platform: <provider>/inventory/collectors/<target_name.rb> <provider>/inventory/parsers/<target_name.rb> <provider>/inventory/target/<target_name.rb> In case the classes are present, targeted refresh JustWorks ™️
2354061
to
ec59992
Compare
Checked commit durandom@ec59992 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
The goal is to make it really simple for authors to create a targeted refresh.
To simplify targeted refresh, we don't differentiate between targeted refresh and full refresh anymore, but a full refresh is simply the manager as a target. So in this ansible example, it's
automation_manager
.Collaborators
Inventory/Collector
A collector acts as a proxy to the providers inventory. It's task is to provide methods that collect this data. It's base class is initialized with
manager
andtarget
, so it can make use of with_provider_connection or restrict collected data to the target scope. Thetarget
in this case is theActiveRecord
instance of the refresh we are targeting. So in a full refresh it's the same as themanager
. If we target aHost
, the target is theHost
instance under themanager
Inventory/Target
This holds all InventoryCollections and makes them accessible via simple methods. Thats needed for cross referencing InventoryObjects via lazy_find
Inventory/Parser
The base class holds the above
collector
andinventory/target
. The entry method is parse which fills the inventory collections by making use of thecollector
Inventory
ManagerRefresh::Inventory takes care of wiring up the above 3 objects: Collector, Target and Parser. It's constructed in the platform and the instances of the 3 objects are constructed in the platform as well. It's all happening in ems_refresher_mixin mainly in
parse_targeted_inventory
.So ideally this is hidden from the provider author.
What's required from a provider author?
Basically the author provides these 3 classes, which inherit from their base classes in the platform:
<provider>/inventory/collectors/<target_name.rb>
<provider>/inventory/parsers/<target_name.rb>
<provider>/inventory/target/<target_name.rb>
In case the classes are present, targeted refresh JustWorks ™️
Can we do better?
As the collector is just a proxy, we could re-use it for different targets. So it's something like the broker in vmware. Eg. let's assume
<provider>/inventory/collectors/<target_name.rb>
is not present, then we fall back to<provider>/inventory/collector.rb
We can do some inheritance for the
inventory/target
. The platform knows what Inventory aCloudManager
can potentially provide. OrCloudManager::Vm
. In the end it will mostly just differ in the class_name of the objects. So we inherit just from the platform. @Ladas has a PR in the works Shared definitions of cloud manager targets #13743The parser could of course re-use some logic by extracting it to mixins that are shared across targets - but I guess thats just implementation detail to the provider author
Still the parser could be a bit smarter by e.g. not relying on a
parse
method, but by e.g. calling all methods that are prefixed withparse_
So with all bullet points above applied, adding a new target could be as simple as adding a new parser subclass.
Notes
I don't like the duplicate use of Target as in
Inventory/Target
and theActiveRecord
Target. I was thinking to call the formerInventory
but that also collides withManagerRefresh/Inventory
. But this his hidden somewhat in the platform. Thoughts?!WRT to @agrare comment
Thats right. So either you split them into different mixins (as per above) or maybe we let the
Inventory/Target
drive the parser. Like everyInventoryCollection
defined in the target tries to fill itself via a corresponding method in the parser.