[WIP] [V2V] Add state machine support for collapse snapshots #19139
Conversation
|
@fdupont-redhat How's this look? I wasn't sure what the difference was between It looks to me like the |
|
@miq-bot add_reviewer @fdupont-redhat |
|
@djberg96 'fdupont-redhat' is an invalid reviewer, ignoring... |
|
@fdupont-redhat Updated. Wasn't sure about the userid, so I went went the userid of the migration task. Let me know if that (or anything else) needs changing. |
…od for removing snapshots.
| end | ||
|
|
||
| handover_to_automate | ||
| queue_signal(:poll_conversion) |
There was a problem hiding this comment.
I would expect it to queue_signal(:collapse_snapshots) if the task isn't finished. You probably need to store the task id in the job context.
There was a problem hiding this comment.
I guess I need to understand why this shouldn't be a blocking method, since we can't proceed unless the snapshots have been deleted, can we?
There was a problem hiding this comment.
I personally prefer non blocking methods, because you release some time for other jobs. But I confess that it's an habit from Automate development, rather than factual :)
There was a problem hiding this comment.
@fdupont-redhat Ok, took an initial stab at it. Not sure if that's the right approach or not.
There was a problem hiding this comment.
Doesn't it start a new task on each iteration? Maybe you should store the task id in context and check for its existence. Also, if the task fails, we should abort because snapshots removal is a prerequisite.
There was a problem hiding this comment.
I don't think I'm understanding how you want this written, so I'll let you update it when you're back on Monday. :)
There was a problem hiding this comment.
A proposal is here: https://github.com/fdupont-redhat/manageiq/blob/v2v_state_machine_collapse_snapshots/app/models/infra_conversion_job.rb.
It uses the fact that we have an async task to stay in the 'collapsing_snapshot' state and retry later. We need to store the task id in the job context, so that when we retry we can retrieve the task status and decide what to do: 1. retry if it's not finished, 2. abort if it failed, 3. call next state if it succeeded.
There was a problem hiding this comment.
@fdupont-redhat Looks like you deleted it. :(
|
Checked commits https://github.com/djberg96/manageiq/compare/c1b613e618e957297578a8a0e62261d2cc88f32b~...2e2658ba3fd620ad40308b75c0fd9cdccf92c859 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
|
This pull request is not mergeable. Please rebase and repush. |
|
Closing in favor of #19177 |
For IMS 1.3 we're moving state machine handling from automate into core. For ease of writing and review, we're breaking this down into (hopefully) easily digestible bits, one state at a time. Each PR will ultimately have a corresponding PR in manageiq-content that deletes the relevant bit of code from automate.
Original automate code: https://github.com/ManageIQ/manageiq-content/blob/master/content/automate/ManageIQ/Transformation/Infrastructure/VM/Common.class/__methods__/collapsesnapshots.rb
Since this is the first thing that needs to happen, we're starting with snapshot collapse, i.e. deletion.
WIP for now until I get specs added and task information updated for the sake of the UI team.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1740881
Depends on: https://bugzilla.redhat.com/show_bug.cgi?id=1741179
Will also need #19149