Fixes #34839 - Add support for NVME Controllers

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

@@ -0,0 +1,20 @@
+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
+    volumes = {}
+    ctrls_and_vol[:volumes].each_with_index do |vol, index|
+      volumes[index.to_s] = vol
+    end
+    attrs["nvme_controllers"], attrs["scsi_controllers"] = ctrls_and_vol[:controllers]&.partition { |controller| controller[:type].include?("VirtualNVMEController") }
+    attrs.delete("controllers")
+    attrs["volumes_attributes"] = volumes
+
+    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

@@ -212,7 +212,7 @@ def vm_delete_action(vm, authorizer = nil)
 
   def vsphere_scsi_controllers(compute_resource)
     scsi_controllers = {}
-    compute_resource.scsi_controller_types.each { |type| scsi_controllers[type[:key]] = type[:title] }
+    compute_resource.storage_controller_types.each { |type| scsi_controllers[type[:key]] = type[:title] }
     scsi_controllers
   end
 

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
@@ -638,7 +642,12 @@ def new_interface(attr = { })
     end
 
     def new_volume(attr = { })
-      client.volumes.new attr.merge(:size_gb => 10)
+      {
+        :thin => true,
+        :name => 'Hard disk',
+        :mode => 'persistent',
+        :size_gb => 10,
+      }
     end
 
     def new_cdrom(attr = {})
@@ -694,6 +703,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
 
@@ -732,6 +744,11 @@ def normalize_vm_attrs(vm_attrs)
         [idx.to_s, ctrl]
       end.to_h
 
+      nvme_controllers = vm_attrs['nvme_controllers'] || {}
+      normalized['nvme_controllers'] = nvme_controllers.map.with_index do |ctrl, idx|
+        [idx.to_s, ctrl]
+      end.to_h
+
       stores = datastores
       volumes_attributes = vm_attrs['volumes_attributes'] || {}
       normalized['volumes_attributes'] = volumes_attributes.each_with_object({}) do |(key, vol), volumes|
@@ -809,6 +826,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']

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/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

test/models/compute_resources/vmware_test.rb

@@ -22,15 +22,15 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
     volumes_attributes    = { "new_volumes" => { "size_gb" => "10", "_delete" => ""},
                               "0"           => { "size_gb" => "1",  "_delete" => ""}}
 
-    attrs_in = HashWithIndifferentAccess.new("cpus"                  => "1",
-                                             "interfaces_attributes" => interfaces_attributes,
-                                             "volumes_attributes"    => volumes_attributes)
+    attrs_in = HashWithIndifferentAccess.new("cpus" => "1",
+      "interfaces_attributes" => interfaces_attributes,
+      "volumes_attributes"    => volumes_attributes)
 
-    attrs_parsed = HashWithIndifferentAccess.new("cpus"                  => "1",
-                                                 "interfaces_attributes" => {"new_interfaces" => {"type" => "VirtualE1000", "network" => "Test network", "_delete" => ""},
-                                                                            "0" => {"type" => "VirtualVmxnet3", "network" => "Test network", "_delete" => ""}},
-                                                 "volumes_attributes"    => {"new_volumes" => {"size_gb" => "10", "_delete" => ""},
-                                                                             "0" => {"size_gb" => "1", "_delete" => ""}})
+    attrs_parsed = HashWithIndifferentAccess.new("cpus" => "1",
+      "interfaces_attributes" => {"new_interfaces" => {"type" => "VirtualE1000", "network" => "Test network", "_delete" => ""},
+                                 "0" => {"type" => "VirtualVmxnet3", "network" => "Test network", "_delete" => ""}},
+      "volumes_attributes"    => {"new_volumes" => {"size_gb" => "10", "_delete" => ""},
+                                  "0" => {"size_gb" => "1", "_delete" => ""}})
 
     mock_vm = mock('vm')
     mock_vm.expects(:save).returns(mock_vm)
@@ -267,7 +267,7 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
     end
 
     test "converts empty hash" do
-      assert_equal({}, @cr.parse_args(HashWithIndifferentAccess.new))
+      assert_empty(@cr.parse_args(HashWithIndifferentAccess.new))
     end
 
     test "converts form attrs to fog attrs" do
@@ -411,31 +411,6 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
       @cr.parse_args(attrs_in)
       assert_equal "network-17", attrs_in["interfaces_attributes"]["0"]["network"]
     end
-
-    context 'scsi_controller_type - from hammer' do
-      test 'parse to be a default scsi_controller_type' do
-        attrs_in = HashWithIndifferentAccess.new('scsi_controller_type' => 'ParaVirtualSCSIController')
-        attrs_out = { scsi_controllers: [{ type: 'ParaVirtualSCSIController' }] }
-        assert_equal attrs_out, @cr.parse_args(attrs_in)
-      end
-
-      test 'do not override scsi_controllers if passed' do
-        attrs_in = HashWithIndifferentAccess.new(
-          'scsi_controller_type' => 'ParaVirtualSCSIController',
-          'scsi_controllers' => [{ 'type' => 'VirtualBusLogicController' }]
-        )
-        attrs_out = { scsi_controllers: [{ type: 'VirtualBusLogicController' }] }
-        assert_equal attrs_out, @cr.parse_args(attrs_in)
-      end
-
-      test 'drop invalid scsi_controller_type attribute' do
-        attrs_in = HashWithIndifferentAccess.new(
-          'scsi_controller_type' => 'ParaVirtualSCSICntrlr'
-        )
-        attrs_out = {}
-        assert_equal attrs_out, @cr.parse_args(attrs_in)
-      end
-    end
   end
 
   describe "#parse_networks" do
@@ -459,7 +434,7 @@ def mock_network(id, name, virtualswitch = nil)
     end
 
     test "converts empty hash" do
-      assert_equal({}, @cr.parse_networks(HashWithIndifferentAccess.new))
+      assert_empty(@cr.parse_networks(HashWithIndifferentAccess.new))
     end
 
     test "converts form network name to network ID" do
@@ -596,6 +571,9 @@ def mock_network(id, name, virtualswitch = nil)
       scsi_controller1 = mock('scsi_controller1')
       scsi_controller1.stubs(:attributes).returns({:type => "VirtualLsiLogicController", :shared_bus => "noSharing", :unit_number => 7, :key => 1000})
       @vm.stubs(:scsi_controllers).returns([scsi_controller1])
+      nvme_controller1 = mock('nvme_controller1')
+      nvme_controller1.stubs(:attributes).returns({:type => "VirtualNVMEController", :key => 2000})
+      @vm.stubs(:nvme_controllers).returns([nvme_controller1])
 
       @networks = [
         OpenStruct.new(:id => 'dvportgroup-123456', :name => 'Testnetwork'),
@@ -619,7 +597,14 @@ def mock_network(id, name, virtualswitch = nil)
             :key => 1000,
           },
         ],
+        :nvme_controllers => [
+          {
+            :type => "VirtualNVMEController",
+            :key => 2000,
+          },
+        ],
       }
+
       attrs = @cr.vm_compute_attributes_for('abc')
 
       assert_equal expected_attrs, attrs
@@ -969,6 +954,33 @@ def mock_network(id, name, virtualswitch = nil)
       assert_equal(expected_attrs, normalized['scsi_controllers'])
     end
 
+    test 'normalizes nvme_controllers' do
+      vm_attrs = {
+        'nvme_controllers' => [
+          {
+            'type' => 'VirtualNVMEController',
+            'key' => 2000,
+          }, {
+            'type' => 'VirtualNVMEController',
+            'key' => 2001,
+          }
+        ],
+      }
+      expected_attrs = {
+        '0' => {
+          'type' => 'VirtualNVMEController',
+          'key' => 2000,
+        },
+        '1' => {
+          'type' => 'VirtualNVMEController',
+          'key' => 2001,
+        },
+      }
+      normalized = cr.normalize_vm_attrs(vm_attrs)
+
+      assert_equal(expected_attrs, normalized['nvme_controllers'])
+    end
+
     test 'normalizes volumes_attributes' do
       vm_attrs = {
         'volumes_attributes' => {
@@ -1046,6 +1058,7 @@ def mock_network(id, name, virtualswitch = nil)
         'memory_hot_add_enabled' => nil,
         'cpu_hot_add_enabled' => nil,
         'scsi_controllers' => {},
+        'nvme_controllers' => {},
         'interfaces_attributes' => {},
         'volumes_attributes' => {},
       }

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/__tests__/StorageContainer.fixtures.js

@@ -56,7 +56,7 @@ export const vmwareData = {
 };
 
 export const hiddenFieldValue = {
-  scsiControllers: [{ key: 1000, type: 'VirtualLsiLogicController' }],
+  controllers: [{ key: 1000, type: 'VirtualLsiLogicController' }],
   volumes: [
     {
       controllerKey: 1000,