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

[V2V] lookup active tasks in a single query #18876

Closed
wants to merge 3 commits into from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jun 14, 2019

Overview

infrastructure conversion distributes work among nodes.
It does this by assigning tasks (the work) to the nodes with the least number of tasks.

refs:

Before

We looked up the number of tasks (aka amount of work) being performed on each conversion host up front for each ems.

This means when we were assigning tasks, we never looked up the task count again, and the numbers never increased.
So the hosts with the least amount of work, still looked like they had the least amount of work.
A single host would get all the tasks for each timer event / round. Besides not distributing the work well, we also sometimes went over the max thresholds for hosts.

After

We are looking up the eligible servers up front for each ems. This work is expensive and it is good to cache.

For each task we are assigning, we are re-calculating the number of tasks running for each conversion host in a single query.
Now we are able to see the increased work loads and we no longer assign all current work to the same conversion host.

@kbrock kbrock added the bug label Jun 14, 2019
@kbrock kbrock requested a review from djberg96 June 14, 2019 13:58
instead of calculating the size in an n+1 manner, use the virtual attributes

Ems#conversion_hosts is not a real collection, so we have to sum
across each collection and add those together
trivial refactor

storing a single conversion_host rather than all of them
remove totals lookup for eligible?
cache eligible_conversion_hosts for each ems

lookup all totals at a time, and select the best host for the job
(the one with the fewest number of active tasks)
@kbrock kbrock changed the title [v2v] lookup active tasks in a single query [V2V] lookup active tasks in a single query Jun 14, 2019
@miq-bot
Copy link
Member

miq-bot commented Jun 14, 2019

Checked commits kbrock/manageiq@6cdfa6c~...484f37b with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 3 offenses detected

lib/infra_conversion_throttler.rb

def total_active_tasks
host_conversion_hosts.sum(:total_active_tasks) + vm_conversion_hosts.sum(:total_active_tasks)
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is on the EMS model, and since other models could potentially need their own list of active tasks, perhaps it would be best to rename this to total_active_conversion_host_tasks.

def total_active_tasks
host_conversion_hosts.sum(:total_active_tasks) + vm_conversion_hosts.sum(:total_active_tasks)
end

Copy link
Member

Choose a reason for hiding this comment

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

This just points out the difficulty I'm going to have trying to move the V2V model code - specifically the conversion_hosts model - out of the core repo and into the V2V repo. Still open to suggestions on how this can be done...

@kbrock
Copy link
Member Author

kbrock commented Jun 14, 2019

this is requiring too much work with the specs. not a fan of all the stubbing.

Something odd was happening. some caching was getting introduced and the counts were not updating.

This can be used for references, but we're avoiding this change for now

@kbrock kbrock closed this Jun 14, 2019
@kbrock kbrock deleted the total_active_task branch June 14, 2019 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants