Show correct VMs upon Service retirement #4710

miha-plesko · 2018-09-27T13:30:52Z

With this commit we make sure correct VMs are shown as part of service retirement request. Talking about "Affected VMs" section that now looks like:

Prior to this commit there was always only a single VM displayed in there, and it was not the right one. Happened to see AWS EC2 VM on a vCloud vApp retirement request details!

The reason for all this was that

@miq_request.options[:src_ids]

was treated as it's a list of VM ids whereas it infact was a list of Service ids. If we were lucky enough, some Service id collided with random VM id and BUUUM, wrong VM was shown.

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

Related PRs:

@miq-bot assign @mzazrivec
@miq-bot add_label enhancement,gaprindashvili/yes,hammer/yes

/cc @tinaafitz @d-m-u

JPrause · 2018-09-28T13:48:47Z

@miq-bot add_label blocker

d-m-u · 2018-09-28T14:31:57Z

app/controllers/application_controller/miq_request_methods.rb

+    elsif @miq_request.resource_type == 'ServiceRetireRequest'
+      @view, @pages = nil
+      if (service_id = @miq_request.options[:src_ids].first) && (service = Service.find_by(:id => service_id)) && Rbac.filtered_object(service)
+        @view, @pages = get_view(Vm, :parent => service, :view_suffix => "VmReconfigureRequest")


I agree that this is a bug, I am however unsure of why VmReconfigureRequests would come into play here, as @tinaafitz pointed out.

AFAIK it's only a matter of what columns are displayed for each Vm. Here I reused the reconfigure view which shows CPU and memory resources consumed by each Vm.

But let me double check on that.

@d-m-u just realized view_suffix has to do with Rbac view, not the haml views. I do apologize for screwing this up, did it right this time. Thanks for pointing out!

mzazrivec · 2018-10-01T10:32:04Z

@d-m-u Do you approve the current fix?

miha-plesko · 2018-10-01T10:57:52Z

@mzazrivec think I should try to come up with some specs? I think there are non currently, but should you find it valuable, I can give it a try.

mzazrivec · 2018-10-01T10:59:43Z

@miha-plesko specs are always appreciated.

miha-plesko · 2018-10-02T10:12:43Z

@mzazrivec pulled half of my hair implementing the spec, but here it is ;) Problem is that list of affected VMs is in fact loaded by Angular not Rails and Rails doesn't even know what VMs will be listed. The rspec we have now is a two-step: first it asserts payload sent from Rails to Angular, then it simulates XHR request with this same payload as Angular would do it, and asserts that we get correct VMs.

Is there a better way? It really got me curious after the hard time I had to implement it at least this way.

mzazrivec · 2018-10-02T10:18:02Z

@skateman Could you please look at the spec and / or give advice? Thanks.

skateman · 2018-10-02T11:25:10Z

spec/factories/miq_request.rb

@@ -0,0 +1,5 @@
+FactoryGirl.define do


Factories should live in the core repo to avoid conflicts. But you probably won't need it anyway, there's some ✨ magic ✨ around them that creates them on-the-fly if they're missing. Just pass these arguments to the .create.

skateman · 2018-10-02T11:26:23Z

spec/controllers/miq_request_controller_spec.rb

+      expect(response.status).to eq(200)
+
+      # Verify Angular got correct VMs when sending the payload as set by previous test.
+      obtained = JSON.parse(response.body)


YAGNI -> response.parsed_body

Dunno, .parsed_body is string for me no matter what headers I set. Even

post :report_data, :params => payload, :format => :json

won't help.. I see other people also need workaround to get this working rails-api/active_model_serializers#1437 (comment)

skateman · 2018-10-02T11:30:55Z

spec/controllers/miq_request_controller_spec.rb

+    end
+
+    it 'angular for grid with affected VMs is properly initialized' do
+      payload = angular_payload.update(:report_data_additional_options => angular_additional)


I'd rather extract this into contexts and then you don't have to call the update:

let(payload) { {:model => blah, additional_key => additional_val} } let(:additional_val) { ... } context 'first' do let(:additional_key) { :report_data_additional_options } it ... end context 'second' do let(:additional_key) { :additional_options } it ... end

That's really smart 👍

miha-plesko · 2018-10-02T11:40:43Z

Thanks @skateman, appreciate your feedback. Is there perhaps a way I could let the Angular to be actually executed on the page returned by the first test, so I wouldn't need to simulate the second test at all (the POST request is actually performed by Angular and I need to be sure that correct list is returned)?

I suppose I want it on too high level so it's a matter of integration tests, but at least I have to ask :)

skateman · 2018-10-02T11:58:32Z

@miha-plesko no, the only thing you can test is the request/response for the GTL data.

With this commit we make sure correct VMs are shown as part of service retirement request. Talking about "Affected VMs" section. Prior to this commit there was always only a single VM displayed in there, and it was not the right one. Happened to see AWS EC2 VM on a vCloud vApp retirement request details! The reason for all this was that ``` @miq_request.options[:src_ids] ``` were treated as it's a list of VM ids whereas it infact was a list of Service ids. If we were lucky enough, some Service id collided with random VM id and BUUUM, wrong VM was shown. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1632239 Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miha-plesko

Okay, so specs are updated, looking forward to any other suggestion.

miha-plesko · 2018-10-02T13:22:46Z

spec/controllers/miq_request_controller_spec.rb

+    end
+
+    it 'angular for grid with affected VMs is properly initialized' do
+      payload = angular_payload.update(:report_data_additional_options => angular_additional)


That's really smart 👍

miha-plesko · 2018-10-02T13:35:40Z

spec/controllers/miq_request_controller_spec.rb

+      expect(response.status).to eq(200)
+
+      # Verify Angular got correct VMs when sending the payload as set by previous test.
+      obtained = JSON.parse(response.body)


Dunno, .parsed_body is string for me no matter what headers I set. Even

post :report_data, :params => payload, :format => :json

won't help.. I see other people also need workaround to get this working rails-api/active_model_serializers#1437 (comment)

miq-bot · 2018-10-02T13:52:30Z

Some comments on commit miha-plesko@80d3684

spec/controllers/miq_request_controller_spec.rb

⚠️ - 305 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2018-10-02T13:52:46Z

Checked commit miha-plesko@80d3684 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

h-kataria · 2018-10-04T16:48:39Z

@skateman @mzazrivec is this ready for merge?

mzazrivec · 2018-10-05T13:20:35Z

@skateman Are you OK with the current specs?

skateman

Show correct VMs upon Service retirement (cherry picked from commit c0c060d) https://bugzilla.redhat.com/show_bug.cgi?id=1632239

simaishi · 2018-10-08T12:41:22Z

Hammer backport details:

$ git log -1
commit b6a3763e1a5d5382ccc4e9fdf15f8dc492691855
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Mon Oct 8 08:58:32 2018 +0200

    Merge pull request #4710 from miha-plesko/retire-request-show-vms
    
    Show correct VMs upon Service retirement
    
    (cherry picked from commit c0c060d2d8bfdb303f54f45a849afc94a21d9fe1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1632239

Show correct VMs upon Service retirement (cherry picked from commit c0c060d) https://bugzilla.redhat.com/show_bug.cgi?id=1636494

simaishi · 2018-10-11T14:15:51Z

Gaprindashvili backport details:

$ git log -1
commit 2e85ec094ec9fbd6cc55bdaffba5727e972da371
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Mon Oct 8 08:58:32 2018 +0200

    Merge pull request #4710 from miha-plesko/retire-request-show-vms
    
    Show correct VMs upon Service retirement
    
    (cherry picked from commit c0c060d2d8bfdb303f54f45a849afc94a21d9fe1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1636494

simaishi · 2018-10-11T15:02:54Z

@miha-plesko Please take a look at Travis failure for Gaprindashvili branch:

https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/440166352

  1) MiqRequestController showing details of a retirement task angular for grid with affected VMs gets the correct VMs with XHR call 
     Failure/Error: @user = stub_admin
     NameError:
       undefined local variable or method `stub_admin' for #<RSpec::ExampleGroups::MiqRequestController_2::ShowingDetailsOfARetirementTask::AngularForGridWithAffectedVMsGetsTheCorrectVMsWithXHRCall:0x00000039c863b8>
     # ./spec/controllers/miq_request_controller_spec.rb:262:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:73:in `block (3 levels) in <top (required)>'
     # ./spec/manageiq/spec/support/evm_spec_helper.rb:35:in `clear_caches'
     # ./spec/spec_helper.rb:73:in `block (2 levels) in <top (required)>'

  2) MiqRequestController showing details of a retirement task angular for grid with affected VMs is properly initialized 
     Failure/Error: @user = stub_admin
     NameError:
       undefined local variable or method `stub_admin' for #<RSpec::ExampleGroups::MiqRequestController_2::ShowingDetailsOfARetirementTask::AngularForGridWithAffectedVMsIsProperlyInitialized:0x0000003c7d52d0>
     # ./spec/controllers/miq_request_controller_spec.rb:262:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:73:in `block (3 levels) in <top (required)>'
     # ./spec/manageiq/spec/support/evm_spec_helper.rb:35:in `clear_caches'
     # ./spec/spec_helper.rb:73:in `block (2 levels) in <top (required)>'

d-m-u · 2018-10-11T15:51:17Z

I also think this shouldn't be in G at all...

d-m-u · 2018-10-11T15:53:51Z

But it'll probably be a valid bug on 5.9, just not anything to do with requests.

d-m-u · 2018-10-11T15:54:25Z

I think we should recheck what this looks like on 5.9.

…-vms" This reverts commit 2e85ec0. https://bugzilla.redhat.com/show_bug.cgi?id=1636494

simaishi · 2018-10-11T17:22:50Z

Reverted the G backport.

commit 2b18e5a19bdfdd52f16924f953c1628565e2e99b
Author: Satoe Imaishi <simaishi@redhat.com>
Date:   Thu Oct 11 13:21:42 2018 -0400

    Revert "Merge pull request #4710 from miha-plesko/retire-request-show-vms"
    
    This reverts commit 2e85ec094ec9fbd6cc55bdaffba5727e972da371.
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1636494

miq-bot assigned mzazrivec Sep 27, 2018

miq-bot added enhancement gaprindashvili/yes labels Sep 27, 2018

miha-plesko force-pushed the retire-request-show-vms branch from cb03822 to 1cc880d Compare September 27, 2018 13:44

miha-plesko mentioned this pull request Sep 27, 2018

Properly serialize OrchestrationStack class name for MiqRetireTask.request_type ManageIQ/manageiq#18023

Merged

miq-bot added the blocker label Sep 28, 2018

d-m-u reviewed Sep 28, 2018

View reviewed changes

miha-plesko force-pushed the retire-request-show-vms branch from 1cc880d to b7355f7 Compare September 28, 2018 15:57

mzazrivec added compute/infrastructure bug and removed enhancement labels Oct 1, 2018

miha-plesko changed the title ~~Show correct VMs upon Service retirement~~ [WIP] Show correct VMs upon Service retirement Oct 1, 2018

miq-bot added the wip label Oct 1, 2018

miha-plesko force-pushed the retire-request-show-vms branch from b7355f7 to 800be06 Compare October 2, 2018 10:05

miha-plesko changed the title ~~[WIP] Show correct VMs upon Service retirement~~ Show correct VMs upon Service retirement Oct 2, 2018

miq-bot removed the wip label Oct 2, 2018

skateman suggested changes Oct 2, 2018

View reviewed changes

miha-plesko force-pushed the retire-request-show-vms branch from 800be06 to 80d3684 Compare October 2, 2018 13:47

miha-plesko commented Oct 2, 2018

View reviewed changes

skateman approved these changes Oct 7, 2018

View reviewed changes

mzazrivec added this to the Sprint 96 Ending Oct 8, 2018 milestone Oct 8, 2018

mzazrivec merged commit c0c060d into ManageIQ:master Oct 8, 2018

simaishi pushed a commit that referenced this pull request Oct 8, 2018

Merge pull request #4710 from miha-plesko/retire-request-show-vms

b6a3763

Show correct VMs upon Service retirement (cherry picked from commit c0c060d) https://bugzilla.redhat.com/show_bug.cgi?id=1632239

simaishi added hammer/backported and removed hammer/yes labels Oct 8, 2018

simaishi pushed a commit that referenced this pull request Oct 11, 2018

Merge pull request #4710 from miha-plesko/retire-request-show-vms

2e85ec0

Show correct VMs upon Service retirement (cherry picked from commit c0c060d) https://bugzilla.redhat.com/show_bug.cgi?id=1636494

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Oct 11, 2018

simaishi added a commit that referenced this pull request Oct 11, 2018

Revert "Merge pull request #4710 from miha-plesko/retire-request-show…

2b18e5a

…-vms" This reverts commit 2e85ec0. https://bugzilla.redhat.com/show_bug.cgi?id=1636494

simaishi added gaprindashvili/no and removed gaprindashvili/backported labels Oct 11, 2018

miha-plesko deleted the retire-request-show-vms branch January 7, 2019 08:24

miha-plesko mentioned this pull request Apr 16, 2019

Show affected servers on PhysicalServerProvisionRequest details #5461

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show correct VMs upon Service retirement #4710

Show correct VMs upon Service retirement #4710

miha-plesko commented Sep 27, 2018

JPrause commented Sep 28, 2018

d-m-u Sep 28, 2018

miha-plesko Sep 28, 2018

miha-plesko Sep 28, 2018

mzazrivec commented Oct 1, 2018

miha-plesko commented Oct 1, 2018

mzazrivec commented Oct 1, 2018

miha-plesko commented Oct 2, 2018

mzazrivec commented Oct 2, 2018

skateman Oct 2, 2018

skateman Oct 2, 2018

miha-plesko Oct 2, 2018

skateman Oct 2, 2018

miha-plesko Oct 2, 2018

miha-plesko commented Oct 2, 2018

skateman commented Oct 2, 2018

miha-plesko left a comment

miha-plesko Oct 2, 2018

miha-plesko Oct 2, 2018

miq-bot commented Oct 2, 2018

miq-bot commented Oct 2, 2018

h-kataria commented Oct 4, 2018

mzazrivec commented Oct 5, 2018

skateman left a comment

simaishi commented Oct 8, 2018 •

edited

Loading

simaishi commented Oct 11, 2018

simaishi commented Oct 11, 2018

d-m-u commented Oct 11, 2018

d-m-u commented Oct 11, 2018

d-m-u commented Oct 11, 2018

simaishi commented Oct 11, 2018

Show correct VMs upon Service retirement #4710

Show correct VMs upon Service retirement #4710

Conversation

miha-plesko commented Sep 27, 2018

JPrause commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzazrivec commented Oct 1, 2018

miha-plesko commented Oct 1, 2018

mzazrivec commented Oct 1, 2018

miha-plesko commented Oct 2, 2018

mzazrivec commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Oct 2, 2018

skateman commented Oct 2, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 2, 2018

miq-bot commented Oct 2, 2018

h-kataria commented Oct 4, 2018

mzazrivec commented Oct 5, 2018

skateman left a comment

Choose a reason for hiding this comment

simaishi commented Oct 8, 2018 • edited Loading

simaishi commented Oct 11, 2018

simaishi commented Oct 11, 2018

d-m-u commented Oct 11, 2018

d-m-u commented Oct 11, 2018

d-m-u commented Oct 11, 2018

simaishi commented Oct 11, 2018

simaishi commented Oct 8, 2018 •

edited

Loading