Fixes #19325 - Adding VMware vSphere SCSI Controller ID Selection to volumes #4471

sbernhard · 2017-04-20T09:00:15Z

Adding VMware vSphere SCSI Controller ID Selection to volumes

theforeman-bot · 2017-04-20T09:00:19Z

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

theforeman-bot · 2017-04-20T09:00:19Z

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

theforeman-bot · 2017-04-20T09:00:19Z

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

mention-bot · 2017-04-20T09:00:21Z

@sbernhard, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ohadlevy, @timogoebel and @mmoll to be potential reviewers.

theforeman-bot · 2017-04-20T09:00:22Z

There were the following issues with the commit message:

5fbb00a must be in the format fixes #redmine_number - brief description
length of the first commit message line for 5fbb00a exceeds 65 characters
commit message for 5fbb00a is not wrapped at 72nd column
f7d4b42 must be in the format fixes #redmine_number - brief description
bfd187e must be in the format fixes #redmine_number - brief description
3e16400 must be in the format fixes #redmine_number - brief description
f448cab must be in the format fixes #redmine_number - brief description
1eeb78f must be in the format fixes #redmine_number - brief description
b9a9404 must be in the format fixes #redmine_number - brief description
ccb4e08 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

timogoebel · 2017-04-20T12:08:32Z

@sbernhard : Thanks. Could you please rebase this and squash it into a single commit?

timogoebel

@sbernhard : Thanks. I left some comments inline. I'm not really convinced, this adds any actual value for customers as long as it's only possible to manage a single scsi controller with foreman. There is a PR open to fix this. #4366

timogoebel · 2017-04-20T12:09:06Z

app/views/compute_resources_vms/form/vmware/_volume.html.erb

@@ -24,3 +24,4 @@
           "true",
           "false" %>
 <%= select_f f, :mode, compute_resource.disk_mode_types, :first, :last, { }, :class => "span5", :label => _("Disk mode"), :label_size => "col-md-2", :disabled => !new_host %>
+<%= select_f f, :controller_key, compute_resource.controller_ids, :first, :last, { }, :class => "span5", :label => _("SCSI Controller ID"), :label_size => "col-md-2" , :disabled => !new_host %>


This should be :disabled => !new_vm. Dito in the line above.

Some fields are with new_host and some with new_vm. I can change it to new_vm if you are sure that this is the right one.

I'm positive new_vm is the correct choice here.

In the current github develop branch there are 2 lines of code which are using "new_host" instead of "new_vm". It's the vmware name and the disk mode. Should I open a new redmine issue and fix both?

Yes, that would be wonderful. Everything we can get into core will be greatly appreciated. We (read: I) renamed new_host to new_vm as 637da2f allows you to create a host in foreman on an existing vm. So the naming was wrong.

See Redmine #19340

timogoebel · 2017-04-20T12:11:33Z

app/models/compute_resources/foreman/model/vmware.rb

@@ -575,7 +594,6 @@ def vm_instance_defaults
        :memory_mb  => 768,
        :interfaces => [new_interface],
        :volumes    => [new_volume],
-        :scsi_controller => { :type => scsi_controller_default_type },


Why remove this?

I'm setting the default type in parse_args as its necessary to set the default controller type for all volumes and not only for one.

timogoebel · 2017-04-20T12:13:03Z

app/models/compute_resources/foreman/model/vmware.rb

+      scsi_controllers = Hash.new
+      args[:volumes_attributes].each do |k,v|
+        Rails.logger.info("Will add #{v[:controller_key]} vmware controller with type #{scsi_controller_type}")
+        scsi_controllers[v[:controller_key]] = {:type => scsi_controller_type, :key => v[:controller_key]}


Does this support API requests where :controller_key is not set?

Didn't tried that right now but from the fog-vsphere code I would say, yes.

timogoebel · 2017-04-20T12:18:03Z

app/models/compute_resources/foreman/model/vmware.rb

+      end
+
+      scsi_controllers = Hash.new
+      args[:volumes_attributes].each do |k,v|


This gives me an NoMethodError: undefined method each' for nil:NilClass` when trying to create a new vm though the UI.

theforeman-bot · 2017-04-20T12:50:14Z

There were the following issues with the commit message:

5fbb00a must be in the format fixes #redmine_number - brief description
length of the first commit message line for 5fbb00a exceeds 65 characters
commit message for 5fbb00a is not wrapped at 72nd column
f7d4b42 must be in the format fixes #redmine_number - brief description
bfd187e must be in the format fixes #redmine_number - brief description
3e16400 must be in the format fixes #redmine_number - brief description
f448cab must be in the format fixes #redmine_number - brief description
1eeb78f must be in the format fixes #redmine_number - brief description
b9a9404 must be in the format fixes #redmine_number - brief description
ccb4e08 must be in the format fixes #redmine_number - brief description
9c2cab1 must be in the format fixes #redmine_number - brief description
281dafc must be in the format fixes #redmine_number - brief description
length of the first commit message line for 281dafc exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

sbernhard · 2017-04-20T13:04:55Z

The main reason for this change is this (page 11):
http://www.vmware.com/files/pdf/solutions/sap/sap-solutions-on-vmware-best-practices-guide.pdf

[cite]
Use multiple virtual SCSI controllers for database virtual machines. The use of multiple virtual SCSI
controllers allows the execution of several parallel I/O operations inside the guest OS. It is
recommended to use one
[/cite]

timogoebel · 2017-04-21T07:30:50Z

@sbernhard : Thanks. I am familiar with that document and that recommendation. That's one reason why we started with #4366. That PR allows a user to add more then one scsi controller to a VM and attach disks to the controllers.

sbernhard · 2017-04-21T07:45:27Z

@timogoebel OK. Then we are working on the same issue from different directions I guess. :-)
Adding SCSI controllers to a VM and then using them in the volume section would be better of course. From the working going on in #4366 you are planning to do this in this way. Therefore, I would be OK to stop working on the #4471 and wait for #4366. Do you agree?

timogoebel · 2017-04-21T07:46:59Z

Therefore, I would be OK to stop working on the #4471 and wait for #4366. Do you agree?

Yes. That would probably be the best approach.

sbernhard · 2017-04-21T08:02:54Z

Can you then close http://projects.theforeman.org/issues/19325 please?

timogoebel · 2017-04-21T08:10:16Z

@sbernhard : I'd prefer to keep the issue open as #4366 does not allow to actually change the scsi id of a controller.

theforeman-bot · 2017-04-21T14:18:50Z

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

theforeman-bot · 2017-04-21T14:18:50Z

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

theforeman-bot · 2017-04-21T14:18:51Z

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Apr 20, 2017

sbernhard changed the title ~~Fixes #19325~~ Fixes #19325 - Adding VMware vSphere SCSI Controller ID Selection to volumes Apr 20, 2017

timogoebel requested changes Apr 20, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Apr 20, 2017

timogoebel removed the Needs testing label Apr 20, 2017

theforeman-bot removed the Waiting on contributor label Apr 20, 2017

theforeman-bot added Needs re-review Needs testing Waiting on contributor labels Apr 20, 2017

sbernhard closed this Apr 21, 2017

sbernhard force-pushed the develop branch from 281dafc to 48c0d61 Compare April 21, 2017 14:18

theforeman-bot removed the Waiting on contributor label Apr 21, 2017

theforeman-bot added Needs re-review Needs testing labels Apr 21, 2017

sbernhard mentioned this pull request Apr 21, 2017

Fixes #19325 - Adding VMware SCSI Controller ID Selection to volumes #4478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #19325 - Adding VMware vSphere SCSI Controller ID Selection to volumes #4471

Fixes #19325 - Adding VMware vSphere SCSI Controller ID Selection to volumes #4471

sbernhard commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

mention-bot commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

timogoebel commented Apr 20, 2017

timogoebel left a comment

timogoebel Apr 20, 2017

sbernhard Apr 20, 2017

timogoebel Apr 20, 2017

sbernhard Apr 20, 2017

timogoebel Apr 21, 2017

sbernhard Apr 21, 2017

timogoebel Apr 20, 2017

sbernhard Apr 20, 2017

timogoebel Apr 20, 2017

sbernhard Apr 20, 2017

timogoebel Apr 20, 2017

theforeman-bot commented Apr 20, 2017

sbernhard commented Apr 20, 2017

timogoebel commented Apr 21, 2017

sbernhard commented Apr 21, 2017

timogoebel commented Apr 21, 2017

sbernhard commented Apr 21, 2017

timogoebel commented Apr 21, 2017

theforeman-bot commented Apr 21, 2017

theforeman-bot commented Apr 21, 2017

theforeman-bot commented Apr 21, 2017

Fixes #19325 - Adding VMware vSphere SCSI Controller ID Selection to volumes #4471

Fixes #19325 - Adding VMware vSphere SCSI Controller ID Selection to volumes #4471

Conversation

sbernhard commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

mention-bot commented Apr 20, 2017

theforeman-bot commented Apr 20, 2017

timogoebel commented Apr 20, 2017

timogoebel left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Apr 20, 2017

sbernhard commented Apr 20, 2017

timogoebel commented Apr 21, 2017

sbernhard commented Apr 21, 2017

timogoebel commented Apr 21, 2017

sbernhard commented Apr 21, 2017

timogoebel commented Apr 21, 2017

theforeman-bot commented Apr 21, 2017

theforeman-bot commented Apr 21, 2017

theforeman-bot commented Apr 21, 2017