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

Use MiqTask for EmsRefresh and remove sync option #14027

Merged
merged 11 commits into from
Mar 9, 2017

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 22, 2017

This wraps the EmsRefresh queue item in an MiqTask so that the task_ids can be aggregated by the caller and MiqTask::wait_for_task_id can be used instead of the current "sync" refresh option which just sleeps on the queue item, blocking refreshes on other EMSs from being queued.

In the event that a queue item gets merged, the original task_id is returned.

Note: this doesn't fix the issue where the worker is blocked from processing other work while waiting for a refresh. We cannot use the queue callbacks to run work after the refresh completes because if a refresh is merged there will be only one queue callback.

@miq-bot
Copy link
Member

miq-bot commented Feb 23, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Feb 23, 2017

This pull request is not mergeable. Please rebase and repush.

@agrare
Copy link
Member Author

agrare commented Mar 2, 2017

Okay @Fryguy I made creation of the task optional, and added a method that will return a task so that if you know you need one you can call EmsRefresh.queue_refresh_task(target) instead of needing EmsRefresh.queue_refresh(target, nil, :create_task => true)

I think we can get rid of the Ansible create_in_provider_queue method completely now but I don't want to do too much else in this PR.

sync ? queue_merge_sync(targets, ems) : queue_merge_async(targets, ems)
end
def self.queue_merge(targets, ems, create_task = false)
task = create_refresh_task(ems, targets) if create_task
Copy link
Member

Choose a reason for hiding this comment

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

Does this end up creating a task even if there is already one on the queue?


# If we merged with an existing queue item we don't need a new
# task, just use the original one
task.delete if task && task_id != task.id
Copy link
Member

Choose a reason for hiding this comment

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

Oh, it's here...wonder if there's a nicer way to do this so as not to create then destroy 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.

Could create the task in the put_or_update block if msg is nil

@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2017

Checked commits agrare/manageiq@4d7b386~...b8c9343 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 0 offenses detected
Everything looks good. 🍪

@Fryguy Fryguy merged commit b03d569 into ManageIQ:master Mar 9, 2017
@Fryguy Fryguy added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 9, 2017
@agrare agrare deleted the ems_refresh_task branch March 9, 2017 16:54
@isimluk
Copy link
Member

isimluk commented Mar 22, 2017

If this gets backported -> then #14441 should be as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants