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.

Sign up for GitHub

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

[V2V] Modify active_tasks so that it always reloads #18860

Merged
merged 1 commit into from
Jun 14, 2019
Merged

[V2V] Modify active_tasks so that it always reloads #18860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion app/models/conversion_host.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,12 @@ def eligible?
# Returns a boolean indicating whether or not the current number of active tasks
# exceeds the maximum number of allowable concurrent tasks specified in settings.
#
# Note that we force a reload of the active tasks via .count because we don't
# want that value cached.
#
def check_concurrent_tasks
max_tasks = max_concurrent_tasks || Settings.transformation.limits.max_concurrent_tasks_per_host
active_tasks.size < max_tasks
active_tasks.count < max_tasks
Copy link
Member

Choose a reason for hiding this comment

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

please introduce total_tasks and reference that here.

that way we can preload this value in this query and not cause an N+1

I understand that you don't want to have a cached value from more than 10 seconds ago, but caching it within a single query / second seems prudent and non-wasteful.

Copy link
Member

Choose a reason for hiding this comment

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

also, of note, this count is checked within a loop that also runs counts.
so a separate count here doesn't make sense.

(also using size and prefetching all active_tasks isn't much better)

Copy link
Member

Choose a reason for hiding this comment

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

consensus: this is bad and should be changed
BUT
not today

==> Keep it with count

end

# Check to see if we can connect to the conversion host using a simple 'uname -a'
Expand Down
13 changes: 11 additions & 2 deletions lib/infra_conversion_throttler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,24 @@
class InfraConversionThrottler
def self.start_conversions
pending_conversion_jobs.each do |ems, jobs|
running = ems.conversion_hosts.inject(0) { |sum, ch| sum + ch.active_tasks.size }
running = ems.conversion_hosts.inject(0) { |sum, ch| sum + ch.active_tasks.count }
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a terrible N+1 (query in a loop is bad)

Copy link
Member

Choose a reason for hiding this comment

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

cc @kbrock

Copy link
Member

Choose a reason for hiding this comment

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

there will be less than 20 conversion hosts.

I did come up with a single query for this, but since it is not in the primary loop, that can hold off for another day. This is only called 1 time per ems and is a lower concern

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it may be tricky treating all conversion_hosts the same.

I'll try and think of a way, but adding together vm_conversion_hosts values and host_conversion_hosts values may be the next best thing.

encapsulating this and putting it into ems may at least make this method look good.

$log&.debug("There are currently #{running} conversion hosts running.")
Copy link
Member

Choose a reason for hiding this comment

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

Why $log&.debug? $log shouldn't be nil. Also, you probably only want to log to debug if the logger is in debug mode, so something like this may be more appropriate..

$log.debug { "There are currently #{running} conversion hosts running." }

Copy link
Member

Choose a reason for hiding this comment

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

yes, please remove this from the PR

slots = (ems.miq_custom_get('Max Transformation Runners') || Settings.transformation.limits.max_concurrent_tasks_per_ems).to_i - running
$log&.debug("The maximum number of concurrent tasks for the EMS is: #{slots}.")
Copy link
Member

Choose a reason for hiding this comment

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

can we remove this from the PR as well?

jobs.each do |job|
eligible_hosts = ems.conversion_hosts.select(&:eligible?).sort_by { |ch| ch.active_tasks.size }
eligible_hosts = ems.conversion_hosts.select(&:eligible?).sort_by { |ch| ch.active_tasks.count }
Copy link
Member

Choose a reason for hiding this comment

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

not a big fan of bringing back every conversion host to then select out the eligible ones.
to then hit the database for each of those.

would like to find a way to get eligible into the query and then possibly do the sum in the database too


if eligible_hosts.size > 0
$log&.debug("The following conversion hosts are currently eligible: " + eligible_hosts.map(&:name).join(', '))
end

break if slots <= 0 || eligible_hosts.empty?
Copy link
Member

Choose a reason for hiding this comment

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

again not you but, if there are no slots, do we even need to do this jobs.each - what will that buy us?
we're going to throw all this work away anyway.

job.migration_task.update_attributes!(:conversion_host => eligible_hosts.first)
Copy link
Member

Choose a reason for hiding this comment

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

I know this is not you but...

do we really need to bring back every eligible host?

if we could get it into the query, could we just bring back the top host and vm and pick one of those?

Copy link
Member

Choose a reason for hiding this comment

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

punting on this idea - looks like it may make sense to just cache the conversion hosts

job.queue_signal(:start)
_log.info("Pending InfraConversionJob: id=#{job.id} signaled to start")
slots -= 1

$log&.debug("The current number of available tasks is: #{slots}.")
end
end
end
Expand Down