Fixes #34839 - Add support for VMware NVME Controllers

app/controllers/concerns/foreman/controller/normalize_vmware_storage_controller_attributes.rb

@@ -0,0 +1,15 @@
+module Foreman::Controller::NormalizeVmwareStorageControllerAttributes
+  extend ActiveSupport::Concern
+
+  private
+
+  def normalize_vmware_storage_controller_attributes(attrs)
+    ctrls_and_vol = JSON.parse(attrs["controllers"]).
+      deep_transform_keys { |key| key.to_s.underscore }.
+      deep_symbolize_keys
+    attrs["volumes_attributes"] = ctrls_and_vol[:volumes].each_with_index.to_h { |vol, index| [index.to_s, vol] }
+    attrs["nvme_controllers"], attrs["scsi_controllers"] = ctrls_and_vol[:controllers]&.partition { |controller| controller[:type].include?("VirtualNVMEController") }
+    attrs.delete("controllers")
+    attrs
+  end
+end

app/controllers/concerns/foreman/controller/parameters/compute_attribute.rb

@@ -1,6 +1,6 @@
 module Foreman::Controller::Parameters::ComputeAttribute
   extend ActiveSupport::Concern
-  include Foreman::Controller::NormalizeScsiAttributes
+  include Foreman::Controller::NormalizeVmwareStorageControllerAttributes
 
   class_methods do
     def compute_attribute_params_filter
@@ -19,8 +19,8 @@ def compute_attribute_params
   def normalized_compute_attribute_params
     normalized = compute_attribute_params
 
-    if normalized["vm_attrs"] && normalized["vm_attrs"]["scsi_controllers"]
-      normalize_scsi_attributes(normalized["vm_attrs"])
+    if normalized["vm_attrs"] && normalized["vm_attrs"]["controllers"]
+      normalize_vmware_storage_controller_attributes(normalized["vm_attrs"])
     end
 
     normalized.to_h

app/controllers/concerns/foreman/controller/parameters/host.rb

@@ -2,7 +2,7 @@ module Foreman::Controller::Parameters::Host
   extend ActiveSupport::Concern
   include Foreman::Controller::Parameters::HostBase
   include Foreman::Controller::Parameters::HostCommon
-  include Foreman::Controller::NormalizeScsiAttributes
+  include Foreman::Controller::NormalizeVmwareStorageControllerAttributes
 
   class_methods do
     def host_params_filter
@@ -33,8 +33,8 @@ def host_params_filter
 
   def host_params(top_level_hash = controller_name.singularize)
     self.class.host_params_filter.filter_params(params, parameter_filter_context, top_level_hash).tap do |normalized|
-      if parameter_filter_context.ui? && normalized["compute_attributes"] && normalized["compute_attributes"]["scsi_controllers"]
-        normalize_scsi_attributes(normalized["compute_attributes"])
+      if parameter_filter_context.ui? && normalized["compute_attributes"] && normalized["compute_attributes"]["controllers"]
+        normalize_vmware_storage_controller_attributes(normalized["compute_attributes"])
       end
     end
   end

app/helpers/compute_resources_vms_helper.rb

@@ -210,12 +210,6 @@ def vm_delete_action(vm, authorizer = nil)
     display_delete_if_authorized(hash_for_compute_resource_vm_path(:compute_resource_id => @compute_resource, :id => vm.identity).merge(:auth_object => @compute_resource, :authorizer => authorizer), :class => 'btn btn-danger')
   end
 
-  def vsphere_scsi_controllers(compute_resource)
-    scsi_controllers = {}
-    compute_resource.scsi_controller_types.each { |type| scsi_controllers[type[:key]] = type[:title] }
-    scsi_controllers
-  end
-
   def new_vm?(host)
     return true unless host.present?
     compute_object = host.compute_object

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

@@ -192,12 +192,13 @@ def nictypes
       }
     end
 
-    def scsi_controller_types
+    def storage_controller_types
       {
         "VirtualBusLogicController" => "Bus Logic Parallel",
         "VirtualLsiLogicController" => "LSI Logic Parallel",
         "VirtualLsiLogicSASController" => "LSI Logic SAS",
         "ParaVirtualSCSIController" => "VMware Paravirtual",
+        "VirtualNVMEController" => "NVME Controller",
       }
     end
 
@@ -482,10 +483,6 @@ def parse_args(args)
         args[collection] = nested_attributes_for(collection, nested_attrs) if nested_attrs
       end
 
-      # see #26402 - consume scsi_controller_type from hammer as a default scsi type
-      scsi_type = args.delete(:scsi_controller_type)
-      args[:scsi_controllers] ||= [{ type: scsi_type }] if scsi_controller_types.key?(scsi_type)
-
       add_cdrom = args.delete(:add_cdrom)
       args[:cdroms] = [new_cdrom] if add_cdrom == '1'
 
@@ -542,8 +539,15 @@ def create_vm(args = { })
       raise e
     end
 
+    def unassigned_volumes?(vols)
+      vols&.any? { |vol| !vol.key?(:controller_key) } || false
+    end
+
     def new_vm(args = {})
       args = parse_args args
+      args = args.deep_symbolize_keys
+      # we will pass empty scsi controllers if the volumes are assigned to nvme controllers to avoid creation of a default scsi controller.
+      args[:scsi_controllers] = [] if !args.key?(:scsi_controllers) && !args[:volumes].empty? && !unassigned_volumes?(args[:volumes])
       opts = vm_instance_defaults.symbolize_keys.merge(args.symbolize_keys).deep_symbolize_keys
       client.servers.new opts
     end
@@ -637,7 +641,7 @@ def new_interface(attr = { })
       client.interfaces.new attr
     end
 
-    def new_volume(attr = { })
+    def new_volume(attr = {})
       client.volumes.new attr.merge(:size_gb => 10)
     end
 
@@ -694,6 +698,9 @@ def vm_compute_attributes(vm)
       vm_attrs[:scsi_controllers] = vm.scsi_controllers.map do |controller|
         controller.attributes
       end
+      vm_attrs[:nvme_controllers] = vm.nvme_controllers.map do |controller|
+        controller.attributes
+      end
       vm_attrs
     end
 
@@ -726,11 +733,15 @@ def normalize_vm_attrs(vm_attrs)
       normalized['add_cdrom'] = to_bool(vm_attrs['add_cdrom'])
 
       normalized['image_name'] = images.find_by(:uuid => vm_attrs['image_id']).try(:name)
-
       scsi_controllers = vm_attrs['scsi_controllers'] || {}
-      normalized['scsi_controllers'] = scsi_controllers.map.with_index do |ctrl, idx|
+      normalized['scsi_controllers'] = scsi_controllers.each_with_index.to_h do |ctrl, idx|
         [idx.to_s, ctrl]
-      end.to_h
+      end
+
+      nvme_controllers = vm_attrs['nvme_controllers'] || {}
+      normalized['nvme_controllers'] = nvme_controllers.each_with_index.to_h do |ctrl, idx|
+        [idx.to_s, ctrl]
+      end
 
       stores = datastores
       volumes_attributes = vm_attrs['volumes_attributes'] || {}
@@ -809,6 +820,7 @@ def vm_instance_defaults
         :interfaces => [new_interface],
         :volumes    => [new_volume],
         :scsi_controllers => [{ :type => scsi_controller_default_type }],
+        :nvme_controllers => [],
         :datacenter => datacenter,
         :firmware => 'automatic',
         :boot_order => ['network', 'disk']
@@ -827,12 +839,12 @@ def set_vm_volumes_attributes(vm, vm_attrs)
     end
 
     def build_vmrc_uri(host, vmid, ticket)
-      uri = URI::Generic.build(:scheme   => 'vmrc',
-                               :userinfo => "clone:#{ticket}",
-                               :host     => host,
-                               :port     => 443,
-                               :path     => '/',
-                               :query    => "moid=#{vmid}").to_s
+      uri = URI::Generic.build(:scheme => 'vmrc',
+        :userinfo => "clone:#{ticket}",
+        :host     => host,
+        :port     => 443,
+        :path     => '/',
+        :query    => "moid=#{vmid}").to_s
       # VMRC doesn't like brackets around IPv6 addresses
       uri.sub(/(.*)\[/, '\1').sub(/(.*)\]/, '\1')
     end

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

@@ -54,13 +54,13 @@ end %>
 <%= react_component('StorageContainer', { data: {
   config: {
     vmExists: !new_vm,
-    controllerTypes: compute_resource.scsi_controller_types,
+    controllerTypes: compute_resource.storage_controller_types,
     diskModeTypes: compute_resource.disk_mode_types,
-    paramsScope: "#{f.object_name}[scsi_controllers]",
+    paramsScope: "#{f.object_name}[controllers]",
     datastoresUrl: available_storage_domains_api_compute_resource_path(compute_resource),
     storagePodsUrl: available_storage_pods_api_compute_resource_path(compute_resource)
   },
   volumes: f.object.volumes.map { |volume| volume.attributes.merge(:size_gb => volume.size_gb).deep_transform_keys { |key| key.to_s.camelize(:lower).to_sym }.reject { |k,v| k == :size } },
-  controllers: f.object.scsi_controllers,
+  controllers: f.object.scsi_controllers + f.object.nvme_controllers,
   cluster: f.object.cluster
 }}) %>

test/controllers/compute_attributes_controller_test.rb

@@ -43,20 +43,21 @@ class ComputeAttributesControllerTest < ActionController::TestCase
     assert_equal "t2.medium", compute_attributes(:one).reload.vm_attrs['flavor_id']
   end
 
-  test "should update compute_attribute with scsi normalization" do
-    json_scsi_data = "{\"scsiControllers\":[{\"type\":\"VirtualLsiLogicController\",\"key\":1000}],\"volumes\":[{\"thin\":true,\"name\":\"Hard disk\",\"mode\":\"persistent\",\"controllerKey\":1000,\"size\":10485760,\"sizeGb\":10,\"storagePod\":\"POD-ZERO\"},{\"sizeGb\":10,\"datastore\":\"\",\"storagePod\":\"POD-ZERO\",\"thin\":false,\"eagerZero\":false,\"name\":\"Hard disk\",\"mode\":\"persistent\",\"controllerKey\":1000}]}"
+  test "should update compute_attribute with controllers normalization" do
+    json_controller_data = "{\"controllers\":[{\"type\":\"VirtualLsiLogicController\",\"key\":1000},{\"type\":\"VirtualNVMEController\",\"key\":2000}],\"volumes\":[{\"thin\":true,\"name\":\"Hard disk\",\"mode\":\"persistent\",\"controllerKey\":1000,\"size\":10485760,\"sizeGb\":10,\"storagePod\":\"POD-ZERO\"},{\"sizeGb\":10,\"datastore\":\"\",\"storagePod\":\"POD-ZERO\",\"thin\":false,\"eagerZero\":false,\"name\":\"Hard disk\",\"mode\":\"persistent\",\"controllerKey\":1000}]}"
     @request.session[:redirect_path] = compute_profile_path(@compute_profile.to_param)
     put :update, params: {
       :id => @set,
       :compute_profile_id => @compute_profile.to_param,
       :compute_attribute => {
         :compute_resource_id => @set.compute_resource_id,
         :compute_profile_id => @set.compute_profile_id,
-        :vm_attrs => {"scsi_controllers" => json_scsi_data},
+        :vm_attrs => {"controllers" => json_controller_data},
       },
     }, session: set_session_user
     saved_attrs = compute_attributes(:one).reload.vm_attrs
     assert_equal [{"type" => "VirtualLsiLogicController", "key" => 1000}], saved_attrs['scsi_controllers']
+    assert_equal [{"type" => "VirtualNVMEController", "key" => 2000}], saved_attrs['nvme_controllers']
     volumes_attrs = {
       '0' => {
         'thin' => true,

test/controllers/concerns/parameters/host_test.rb

@@ -28,14 +28,16 @@ class HostParametersTest < ActiveSupport::TestCase
     assert_equal({'type' => 'awesome', 'network' => 'superawesome'}, filtered['interfaces_attributes'][0]['compute_attributes'].to_h)
   end
 
-  test 'normalizes json scsi attributes' do
-    inner_params = {:name => 'test.example.com', :compute_attributes => {"scsi_controllers" => "{\"scsiControllers\":[{\"type\":\"VirtualLsiLogicController\",\"key\":1000}],\"volumes\":[{\"thin\":true,\"name\":\"Hard disk\",\"mode\":\"persistent\",\"controllerKey\":1000,\"size\":10485760,\"sizeGb\":10,\"storagePod\":\"Example-Pod\"}]}"}}
+  test 'normalizes json controller attributes' do
+    inner_params = {:name => 'test.example.com', :compute_attributes => {"controllers" => "{\"controllers\":[{\"type\":\"VirtualLsiLogicController\",\"key\":1000},{\"type\":\"VirtualNVMEController\",\"key\":2000}],\"volumes\":[{\"thin\":true,\"name\":\"Hard disk\",\"mode\":\"persistent\",\"controllerKey\":1000,\"size\":10485760,\"sizeGb\":10,\"storagePod\":\"Example-Pod\"}]}"}}
     expects(:params).at_least_once.returns(ActionController::Parameters.new(:host => inner_params))
     expects(:parameter_filter_context).at_least_once.returns(ui_context)
     filtered = host_params
 
     assert_equal 'test.example.com', filtered['name']
     assert_equal [{"type" => "VirtualLsiLogicController", "key" => 1000}], filtered['compute_attributes']['scsi_controllers']
+    assert_equal [{"type" => "VirtualNVMEController", "key" => 2000}], filtered['compute_attributes']['nvme_controllers']
+
     assert_equal({"0" => {"thin" => true, "name" => "Hard disk", "mode" => "persistent", "controller_key" => 1000, "size" => 10485760, "size_gb" => 10, "storage_pod" => "Example-Pod"}}, filtered['compute_attributes']['volumes_attributes'])
   end
 end

test/controllers/hosts_controller_test.rb

@@ -1408,7 +1408,7 @@ class Host::Test < Host::Base; end
       teardown { Fog.unmock! }
 
       it 'allows edit disk size' do
-        scsi_controllers = [{ 'type' => 'VirtualLsiLogicController', 'key' => 1000 }]
+        controllers = [{ 'type' => 'VirtualLsiLogicController', 'key' => 1000 }]
         volume_params = {
           'thin' => true,
           'name' => 'Hard disk',
@@ -1424,17 +1424,20 @@ class Host::Test < Host::Base; end
 
         Host::Managed.any_instance.expects('compute_attributes=').with(
           {
-            'scsi_controllers' => scsi_controllers,
+            'scsi_controllers' => controllers,
+            'nvme_controllers' => [],
             'volumes_attributes' => { '0' => volume_attributes },
           }
         )
 
         put :update, params: {
           commit: "Update",
           id: @vmware_host.name,
-          host: { compute_attributes: { scsi_controllers: { 'scsiControllers' => scsi_controllers, 'volumes' => [volume_params] }.to_json } },
+          host: { compute_attributes: { controllers: { 'controllers' => controllers, 'volumes' => [volume_params] }.to_json } },
         }, session: set_session_user
 
+        # require 'pry'
+        # binding.pry
         assert_redirected_to host_details_page_path(@vmware_host.to_param)
       end
     end

test/models/compute_resources/compute_resource_test_helpers.rb

@@ -100,6 +100,7 @@ def allowed_vm_attr_names
       resource_pool_name
       scheduler_hint_filter
       scsi_controllers
+      nvme_controllers
       security_groups
       security_group_id
       security_group_name