Skip to content
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

Move the vmware-specific assign_ems_created_on method #655

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions app/models/manageiq/providers/vmware/infra_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,43 @@ def find_vm_create_events(vms_list)
found
end

def assign_ems_created_on_queue(vm_ids)
MiqQueue.submit_job(
:class_name => self.class.name,
:instance_id => id,
:method_name => 'assign_ems_created_on',
:role => 'ems_operations',
:args => [vm_ids],
:priority => MiqQueue::MIN_PRIORITY
)
end

def assign_ems_created_on(vm_ids)
vms_to_update = vms_and_templates.where(:id => vm_ids, :ems_created_on => nil)
return if vms_to_update.empty?

# 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"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh this is a nasty N+1...I'll put in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was a straight move, refactoring to follow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me wonder if we should just get rid of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, on vSphere 6.7 we can actually pull this from inventory directly (#645) so this won't be needed at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'm good with removing it then, you want to do that in #645 then?

Copy link
Member

@Fryguy Fryguy Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fryguy @agrare if this functionality is new to master, then I am ok with it being available on vSphere 6.7+. If we had this functionality for older vSphere versions, then we need to preserve it, probably by checking for vSphere version and then doing the optimal thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then, I agree to drop it. if a user complains, we can have a conversation at that time about what to do

v.update_attribute(:ems_created_on, event.timestamp) if event && v.ems_created_on != event.timestamp
event
end
return if vms_to_update.empty?

# Of the VMs still without an VM create time, use historical events, if
# available, to determine the VM create time

vms_list = vms_to_update.collect { |v| {:id => v.id, :name => v.name, :uid_ems => v.uid_ems} }
found = find_vm_create_events(vms_list)

# Loop through the found VMs and set their create times
found.each do |vmh|
v = vms_to_update.detect { |vm| vm.id == vmh[:id] }
v.update_attribute(:ems_created_on, vmh[:created_time])
end
end

def get_files_on_datastore(datastore)
with_provider_connection do |vim|
begin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def initialize_inventory_collections
add_collection(infra, :vm_resource_pools)
add_collection(infra, :root_folder_relationship)
add_collection(infra, :orchestration_templates)
vms_and_templates_assign_created_on if ::Settings.ems_refresh.capture_vm_created_on_date
end

def vim_class_to_collection(managed_object)
Expand All @@ -61,4 +62,23 @@ def vim_class_to_collection(managed_object)
resource_pools
end
end

private

def vms_and_templates_assign_created_on
custom_save_block = lambda do |ems, inventory_collection|
vms_and_templates = inventory_collection.dependency_attributes[:vms_and_templates]&.first
return if vms_and_templates.nil?

created_vm_ids = vms_and_templates.created_records.map { |rec| rec[:id] }
ems.assign_ems_created_on_queue(created_vm_ids) unless created_vm_ids.empty?
end

settings = {:without_model_class => true, :auto_inventory_attributes => false}

add_collection(infra, :vms_and_templates_assign_created_on, {}, settings) do |builder|
builder.add_custom_save_block(custom_save_block)
builder.add_dependency_attributes(:vms_and_templates => ->(persister) { [persister.vms_and_templates] })
end
end
end