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

Lookup SCSI Controller Device Type from Hardware [Depends manageiq-gems-pending/148] #51

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented May 2, 2017

When creating a new SCSI controller we used to always default to VirtualLsiLogicController, this isn't ideal since some operating systems depend on a certain type of controller.

This change will lookup the type from the most recent scsi controller when adding a new one

Depends on: ManageIQ/manageiq-gems-pending#148

Here are some screenshots of adding a new controller when the first controller is of type LSI SAS:
Before:
screenshot from 2017-05-03 12-24-05

After:
screenshot from 2017-05-03 12-15-06

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

@agrare agrare changed the title Lookup SCSI Controller Device Type from Hardware Lookup SCSI Controller Device Type from Hardware [Depends manageiq-gems-pending/148] May 2, 2017
@agrare agrare force-pushed the bz_1445874_vm_reconfigure_scsi_controller branch from 46921d1 to e5adccd Compare May 3, 2017 15:49
@agrare agrare requested a review from blomquisg May 3, 2017 16:26
@agrare
Copy link
Member Author

agrare commented May 3, 2017

@blomquisg can you take a look?

@@ -92,16 +92,24 @@ def build_config_spec(options)

if options[:disk_remove] || options[:disk_add]
with_provider_object do |vim_obj|
options[:disk_remove].each { |d| remove_disk_config_spec(vim_obj, vmcs, d) } if options[:disk_remove]
add_disks(vim_obj, vmcs, options[:disk_add]) if options[:disk_add]
hardware = vim_obj.send(:getHardware)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have to send here? Could this just be vim_obj.getHardware?


def add_disks(vim_obj, vmcs, hardware, disks)
available_units = vim_obj.send(:available_scsi_units, hardware)
available_scsi_buses = vim_obj.send(:available_scsi_buses, hardware)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same send question.

@blomquisg
Copy link
Member

Overall it looks fine. Just wonder about the sends because I didn't notice those methods were private on MiqVimVm.

@agrare
Copy link
Member Author

agrare commented May 4, 2017

So unfortunately the answer to the send question is because it was always done that way and I assumed it had something to do with DRb because those methods are being called in the broker.

I tested it out without the sends with a broker worker and its just fine so I'll push a commit to remove send from all the calls in this file.

@agrare agrare force-pushed the bz_1445874_vm_reconfigure_scsi_controller branch from e5adccd to 0039fcd Compare May 4, 2017 18:45
@miq-bot
Copy link
Member

miq-bot commented May 4, 2017

Checked commits agrare/manageiq-providers-vmware@8acb418~...0039fcd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@agrare
Copy link
Member Author

agrare commented May 10, 2017

Just wonder about the sends

Done

@Loicavenel
Copy link

@blomquisg can you merge this one

@blomquisg blomquisg merged commit 774c485 into ManageIQ:master May 17, 2017
@blomquisg blomquisg added this to the Sprint 61 Ending May 22, 2017 milestone May 17, 2017
@simaishi
Copy link
Contributor

@agrare @blomquisg this has been requested to be backported to Euwe (which means also Fine). That would be ok?

@agrare agrare deleted the bz_1445874_vm_reconfigure_scsi_controller branch May 18, 2017 13:04
@agrare
Copy link
Member Author

agrare commented May 18, 2017

@simaishi that shouldn't be a problem

We'll need ManageIQ/manageiq-gems-pending#148 to go back to those branches as well.

@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit 4abb8018fc09c13b6ccf1d8081377b050b26347e
Author: Greg Blomquist <[email protected]>
Date:   Wed May 17 13:03:44 2017 -0400

    Merge pull request #51 from agrare/bz_1445874_vm_reconfigure_scsi_controller
    
    Lookup SCSI Controller Device Type from Hardware [Depends manageiq-gems-pending/148]
    (cherry picked from commit 774c48510e06c093b9366cd848d9bcecf4a885d1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1452172

simaishi pushed a commit that referenced this pull request Jun 8, 2017
…troller

Lookup SCSI Controller Device Type from Hardware [Depends manageiq-gems-pending/148]
(cherry picked from commit 774c485)

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

simaishi commented Jun 8, 2017

Fine backport details:

$ git log -1
commit c54657139c6d621ccf0d166f070ede5822cf5852
Author: Greg Blomquist <[email protected]>
Date:   Wed May 17 13:03:44 2017 -0400

    Merge pull request #51 from agrare/bz_1445874_vm_reconfigure_scsi_controller
    
    Lookup SCSI Controller Device Type from Hardware [Depends manageiq-gems-pending/148]
    (cherry picked from commit 774c48510e06c093b9366cd848d9bcecf4a885d1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1459262

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019
Generic Service State Machine method update.
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.

5 participants