Move the vmware-specific assign_ems_created_on method #655
Conversation
agrare
force-pushed
the
move_vmware_specific_assign_ems_created_on
branch
from
8c51e40 to
570da3c
Compare
The VmOrTemplate.assign_ems_created_on method is very VMware specific, it looks for VMware events then queries the historical event history to find created events.
agrare
force-pushed
the
move_vmware_specific_assign_ems_created_on
branch
from
570da3c to
b64a6a8
Compare
|
Checked commit agrare@b64a6a8 with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint app/models/manageiq/providers/vmware/infra_manager.rb
|
| # Of the VMs without a VM create time, filter out the ones for which we | ||
| # already have a VM create event | ||
| vms_to_update = vms_to_update.reject do |v| | ||
| event = v.ems_events.find_by(:event_type => ["VmCreatedEvent", "VmDeployedEvent"]) |
There was a problem hiding this comment.
Ugh this is a nasty N+1...I'll put in a follow up PR.
There was a problem hiding this comment.
Yeah this was a straight move, refactoring to follow
There was a problem hiding this comment.
Also I should note that this is disabled in Settings by default which is why we probably never noticed when it was in core VmOrTemplate
There was a problem hiding this comment.
Makes me wonder if we should just get rid of it.
There was a problem hiding this comment.
Yeah, on vSphere 6.7 we can actually pull this from inventory directly (#645) so this won't be needed at all.
There was a problem hiding this comment.
👍 I'm good with removing it then, you want to do that in #645 then?
There was a problem hiding this comment.
Would like @chessbyte 's input here as well.
@chessbyte tl;dr In the past vSphere didn't give us the created date, so we guessed based on the events (we'd find a VmCreatedEvent or VmDeployedEvent). However that guessing was behind an option that defaulted to false. Since in vSphere 6.7+ we get it for free from the refresh, I'm proposing dropping this "guessing" code altogether.
There was a problem hiding this comment.
There was a problem hiding this comment.
There is a check at the beginning on the method for whether ems_created_on is already set, so it will naturally skip vsphere 6.7+. I guess where I'm coming from is that if the setting is false by default, then there's likely nobody turning it on. So, the actual usage of this is probably nobody, hence my thought to drop it.
There was a problem hiding this comment.
then, I agree to drop it. if a user complains, we can have a conversation at that time about what to do
The VmOrTemplate.assign_ems_created_on method is very VMware specific, it looks for VMware events then queries the historical event history to find created events.
Also using a graph-refresh dependent collection allows us to skip the step of "finding" vms which were created in this refresh by timestamp since we have access to the set of created_records directly.
ManageIQ/manageiq#19440