-
Notifications
You must be signed in to change notification settings - Fork 898
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
Add VmOrTemplate.remove_all_snapshots_queue #19150
Add VmOrTemplate.remove_all_snapshots_queue #19150
Conversation
@miq-bot add-label enhancement, ivanchuk/yes |
@@ -150,6 +150,18 @@ def remove_all_snapshots | |||
raw_remove_all_snapshots | |||
end | |||
|
|||
def remove_all_snapshots_queue(task_id = nil) | |||
MiqQueue.put_unless_exists( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to get away from put_unless_exists
plus I don't think you want/need this in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you use instead ? put_or_update
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just .put
or even better MiqTask.generic_action_with_callback
handles this for you
:args => [], | ||
:role => "ems_operations", | ||
:zone => my_zone, | ||
:task_id => task_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing a task_id in I think you want to just do MiqTask.generic_action_with_callback
which will create the task and when the miq_queue method finishes it updates the task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see you're following the pattern of the rest of the remove_snapshot_queue
methods, I'm not sure that the task will be updated when the queue item finishes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't think this task_id is what you're thinking it is, https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_action.rb#L635-L640
The task_id for those is task_id = "action_#{action.id}_vm_#{rec.id}"
@agrare I updated with the pattern found in CloudVolume.detach_volume_queue |
Checked commits fabiendupont/manageiq@6e8f23b~...fa38dfe with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 tested, lgtm
…s_queue Add VmOrTemplate.remove_all_snapshots_queue (cherry picked from commit bdf430d) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1767550
Ivanchuk backport details:
|
In existing implementation, it's possible to queue a single snapshot removal via
VmOrTemplate.remove_snapshot_queue
. However, this is not possible to queue the removal of all snapshots, while it's probably going to take longer.This PR introduces
VmOrTemplate.remove_all_snapshots_queue
.RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1741326