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

Properly serialize OrchestrationStack class name for MiqRetireTask.request_type #18023

Merged
merged 1 commit into from
Oct 4, 2018

Conversation

miha-plesko
Copy link
Contributor

@miha-plesko miha-plesko commented Sep 26, 2018

With this commit we make sure retirement request validation succeeds, because right now it's failing with:

[ActiveRecord::RecordInvalid]: Validation failed: OrchestrationStackRetireTask:
    Request type should be orchestration_stack_retire

Reason for the error above ^ is that OrchestrationStack.class.name gets transformed into orchestrationstack while validation expects underscore inbetween: orchestration_stack.

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:

@miha-plesko
Copy link
Contributor Author

Hi guys, lady,
so as discussed two days ago on gitter I'm trying to make VMware vCloud orchestration stack retirement work again. I'm not sure at what point it got broken, but doesn't matter really. I took @d-m-u 's advice and picked better entrypoint for retirement (see this PR). But it didn't work out of the box, hence the two related PRs.

Kindly ask for your opinion - is this the right approach? It seems to be working for me locally.

@tinaafitz
Copy link
Member

@miha-plesko Thanks for creating a PR. 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. We'll be able to provide feedback once we understand the differences.

@JPrause
Copy link
Member

JPrause commented Sep 28, 2018

@miq-bot add_label blocker

@tinaafitz
Copy link
Member

tinaafitz commented Oct 1, 2018

Hi @miha-plesko Thanks for all of your work with Orchestration Stack retirement changes and thanks for catching the validation issue mentioned above.

Along with your validation change, we've identified a few other places where we need to make some minor adjustments to get Orchestration Stack retirement to work properly from Services and stand alone.

The entry point change in this PR is not necessary. The Service retirement entry point should still be the default Service retirement entry point, rather than the Orchestration Stack retirement entry point.

We found a few issues in the automation engine parse_provider_category and parse_automation_request builtin methods that once resolved, construct the correct orchestration stack entry point.

Could you remove the entry point change, and modify the title to describe the validation issue?

With this commit we make sure retirement request validation
succeeds, because right now it's failing with:

```
[ActiveRecord::RecordInvalid]: Validation failed: OrchestrationStackRetireTask:
    Request type should be orchestration_stack_retire
```

Reason for the error above ^ is that `OrchestrationStack.class.name` gets
transformed into `orchestrationstack` while validation expects underscore
inbetween: `orchestration_stack`.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1632239

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
@miha-plesko
Copy link
Contributor Author

Hi @tinaafitz okay, I did as you suggest so we're probably good to merge this one. I assume that at some point, the same retirement StateMachine should be triggered as addressed by ManageIQ/manageiq-content#432, right? Or will the generic StateMachine do the job in its own way?

Thanks for looking at it, looking forward to have it working again!

@miq-bot
Copy link
Member

miq-bot commented Oct 1, 2018

Checked commit miha-plesko@338d348 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@d-m-u
Copy link
Contributor

d-m-u commented Oct 1, 2018

Can the title of this PR be changed to be more relevant, please?

@miha-plesko miha-plesko changed the title Make OrchestrationService retirement work again Properly serialize OrchestrationStack class name for MiqRetireTask.request_type Oct 1, 2018
@bdunne bdunne added bug and removed enhancement labels Oct 4, 2018
@bdunne bdunne merged commit 55fcefa into ManageIQ:master Oct 4, 2018
@bdunne bdunne added this to the Sprint 96 Ending Oct 8, 2018 milestone Oct 4, 2018
@bdunne bdunne assigned bdunne and unassigned tinaafitz Oct 4, 2018
@miha-plesko
Copy link
Contributor Author

@d-m-u @tinaafitz I see a number of PRs have been merged to address orchestration retirement. Kindly ask for a ping when retirement is fixed so I can test for vCloud as well. Thanks :)

simaishi pushed a commit that referenced this pull request Oct 4, 2018
Properly serialize OrchestrationStack class name for MiqRetireTask.request_type

(cherry picked from commit 55fcefa)

https://bugzilla.redhat.com/show_bug.cgi?id=1632239
@simaishi
Copy link
Contributor

simaishi commented Oct 4, 2018

Hammer backport details:

$ git log -1
commit 6e37731fe719b488e5330b077360cc4c7e96306e
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Thu Oct 4 07:15:05 2018 -0400

    Merge pull request #18023 from miha-plesko/retire-orch-stack
    
    Properly serialize OrchestrationStack class name for MiqRetireTask.request_type
    
    (cherry picked from commit 55fcefaf094495201a8cffd32f3d4036615c4e7b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1632239

@tinaafitz
Copy link
Member

hi @miha-plesko, yes, these PRs resolve a few orchestration stack retirement issues:
ManageIQ/manageiq-automation_engine#242
ManageIQ/manageiq-automation_engine#238
I believe there's one issue that has yet to be resolved. We were getting an error while doing orchestration stack retirement. I don't remember if the issue presented as part of Service retirement, or stand alone OrchestrationStack retirement. It would be great if you're able to test again based on the updated code.
I don't believe we'll need the content repo PR, but I'd like to keep it marked as WIP for the time being.
Let me know if you have any questions.

@miha-plesko
Copy link
Contributor Author

@tinaafitz tested on latest master and it's not working for me yet. Attaching Request screenshot as well as my entire evm.log and automation.log. While there seems to be no error in automate.log, the evm.log complains that (L79):

ERROR -- : Q-task_id([r2_orchestration_stack_retire_task_3]) MIQ(MiqAeEngine.deliver) Error delivering {"request"=>"orchestration_stack_retire", "OrchestrationStack::OrchestrationStack"=>4, "User::user"=>1, "OrchestrationStackRetireTask::orchestration_stack_retire_task"=>3} for object [OrchestrationStackRetireTask.3] with state [] to Automate: undefined method `ext_management_system' for #<MiqAeMethodService::MiqAeServiceOrchestrationStackRetireTask:0x00000000124baef8>

image

There are are a couple of issues still present:

  • retirement process does not trigger stack removal from provider. I can see vCloud provider still contains all the workload which means "delete workload" request was never triggered.
  • retirement request still shows incorrect VMs, probably because the ui-classic PR is not merged

PS: I've took latest master and even dropped entire VMDB so I'm 100% it's the latest thing available.

@tinaafitz
Copy link
Member

Hi @miha-plesko Thanks for the detailed feedback. I see that your UI PR is still open, so hopefully that will get merged soon.
We have a 2pm meeting today to discuss the retirement issues. I'll keep you posted on our progress.

@simaishi
Copy link
Contributor

@miha-plesko @tinaafitz app/models/service_retire_task.rb doesn't exist in gaprindashvili branch. Does this change need to go somewhere else, or this PR should be gaprindashvili/no?

@d-m-u
Copy link
Contributor

d-m-u commented Oct 11, 2018

I think this shouldn't be in gaprindashvili at all, since the retire as a request isn't in G.

@d-m-u
Copy link
Contributor

d-m-u commented Oct 11, 2018

@miq-bot add_label gaprindashvili/no
@miq-bot remove_label gaprindashvili/yes

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.

7 participants