[WIP] Allow OrchestrationStack retirement to be triggered from Service #432
Conversation
| @@ -22,7 +32,12 @@ | |||
| exit MIQ_ABORT | |||
| end | |||
|
|
|||
| # Store stack as state var so we needn't search for it every time. | |||
| $evm.set_state_var('stack', stack) | |||
| $evm.set_state_var('stack_service', stack_service) | |||
There was a problem hiding this comment.
Do not store ServiceModel objects in the state_var area. These are not guaranteed to be de-serialized later and with result in errors. Instead, if needed, store the object's ID and do the lookup in the other method.
But I'm not sure much of this is needed.
We are storing the stack_service so we can call start_retirement and finish_retirement but that could just as easily be done with the try command against $evm.root['service'].
$evm.root['service'].try(:start_retirement)
And the lookup of the stack, outside of wanting to log where it comes from, could be:
stack = $evm.root['orchestration_stack'] || $evm.root['service'].orchestration_stack
Again, maybe we can use an embedded method to add logging and load the stack objects to de-dup the code.
@tinaafitz Please review.
cc @d-m-u
There was a problem hiding this comment.
Thanks @gmcculloug. May I also ask for your opinion on entrypoint selection? Was struggling between "service retirement" approach and "orchestration stack retirement approach", picked the latter eventually. It feels like the way to go, but would value your experienced opinion.
This is the PR where I switch entrypoint ManageIQ/manageiq#18023. So on Service UI I now click "Retire this service" which results in the workflow implemented here in the PR to run. Is this OK?
There was a problem hiding this comment.
@miha-plesko Our recent retirement as a request enhancement has changed the way we process retirement. We're looking into how orchestration stacks are retired now vs. how they were handled previously and will report our findings back here.
|
@miq-bot add_label blocker |
miha-plesko
force-pushed
the
allow-stack-retirement-from-service
branch
2 times, most recently
from
a14b2b6 to
636c9c4
Compare
|
@gmcculloug sorry it took me so long to fix it, but here it is. Stack is now retrieved from Workspace using embedded method. I like it much better to have it DRY, so glad you pointed it out. The PR seems bigger but it's just because we have to explicitly register the embedded method everytime we use it. @tinaafitz looking forward to your opinion. |
| handle.log(:info, 'Fetched stack from $evm.object["orchestration_stack_id"]') | ||
| stack | ||
|
|
||
| # From $evm.root. |
There was a problem hiding this comment.
Looks like there is a nice opportunity to refactor this object and root lookup code. I would create another method and pass the object reference (handle.root or handle.object) and maybe a string to help with the logging.
There was a problem hiding this comment.
@gmcculloug yeah was a bit more involved, but I think it's worth it. Only the diff is ugly now, sorry about that. So what I did was:
- move the mixin into
/ManageIQ/Cloud/Orchestration/Lifecycleclass because methods likeget_stackandget_serviceshould probably be consumed by other instances as well, not just Retirement - opt-in for
includesyntax to get rid of the gigantic namespace in front ofget_stack&get_service - convert direct methods into class methods. Or else rubocop has hard time with include in global namespace
- fix the Travis: was failing because the mixin wasn't loaded
Looking forward to your opinion.
Current stack retirement automation assumes being triggered on stack directly, but we want to be able to use the entrypoint ``` /Cloud/Orchestration/Retirement/StateMachines/StackRetirement/Default ``` from the service that handles stack as well. With this commit we therefore adjust existing stack retirement workflow to accept stack passed either as ``` $evm.root['orchestration_stack'] ``` or as ``` $evm.root['service'].orchestration_stack ``` In latter case we also make sure that Service status gets properly updated (so that the service is then moved to "Retired Services" directory). Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1632239 Signed-off-by: Miha Pleško <[email protected]>
miha-plesko
force-pushed
the
allow-stack-retirement-from-service
branch
from
636c9c4 to
cbb676b
Compare
|
Checked commit miha-plesko@cbb676b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
miha-plesko
changed the title
|
Marking WIP for now as @tinaafitz suggests there is a better approach to fix the retirement and this change may not be needed. @tinaafitz please let me know if I can be of any help, we need this for 5.9.5 which is approx end of October. |
|
@miha-plesko @tinaafitz with the final 5.9.5 final build on Oct 15, where does this PR stand? |
|
@JPrause so @tinaafitz took the BZ over and I don't have latest status. @tinaafitz sorry to push you so hard, but is there something I can do to help you with this? We really need it for 5.9.5... |
|
Hi @miha-plesko We can close this PR since content changes are not required. |
Current stack retirement automation assumes being triggered on stack directly, but we want to be able to use the entrypoint
from the service that handles stack as well. With this commit we therefore adjust existing stack retirement workflow to accept stack passed either as
or as
In latter case we also make sure that Service status gets properly updated (so that the service is then moved to "Retired Services" directory).
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1632239
@miq-bot add_label enhancement,gaprindashvili/yes,hammer/yes
@miq-bot assign @tinaafitz
/cc @d-m-u @agrare
Related PRs: