Add support for NVME Controllers #300

girijaasoni · 2024-05-15T00:44:03Z

This is the solution for the feature request.
NVME (Non Volatile Memory Express) controllers are a class above the currently supported (SCSI Controllers) and has multiple advantages

Currently Foreman and fog-vsphere are designed to only support SCSI controllers and it's types. The goal is to extend it to adding NVME and in future, other types. Do give this a read SCSI, SATA, and NVMe Storage Controller Conditions, Limitations, and Compatibility

Front End PR : theforeman/foreman#10168
Hammer doc PR : theforeman/hammer-cli-foreman#628

More details: https://docs.google.com/document/d/1hs4JekeCQP6brbZGg8hS8kcHohselH1HTEEhThgw-AE/edit#heading=h.lrnpszeygrmo

Ready for testing :D

ekohl

For RuboCop it's probably best to disable it in that class.

ekohl · 2024-05-17T07:22:11Z

lib/fog/vsphere/models/compute/server.rb

+          attributes[:volumes]&.map! do |vol|
+            return true unless vol.key?(:controller_key)
+          end
+          false


I wonder why you're using map!. Wouldn't that potentially wipe data? Isn't this what you're looking for?

Suggested change

attributes[:volumes]&.map! do |vol|

return true unless vol.key?(:controller_key)

end

false

attributes[:volumes]&.any? { |vol| !vol.key?(:controller_key) } || false

Technically || false isn't needed since nil and false evaluate to the same, but it ensures a proper boolean is returned.

that's a much better approach, Thanks!

I recommend reading https://docs.ruby-lang.org/en/master/Enumerable.html because it's extremely useful to know. Many things include Enumerable, making your life easier.

ekohl · 2024-05-17T07:24:21Z

lib/fog/vsphere/models/compute/server.rb

+        def nvme_controller
+          nvme_controllers.first
+        end


From reading the above you're adding this, but deprecating it in the same PR. Wouldn't it be better to not introduce it at all?

I suppose we can just not deprecate it. Scsi controllers were deprecated in 60a5dc3#diff-49106de6ade06895f1a509db7c9cd040ed76bfdb472548d8b99efc0468dce548R9 , not sure why
I tried to mirror the behavior but I guess we can undo that. @chris1984 , any idea why scsi_controllers were deprecated

Note it's the singular scsi_controller that's deprecated. IMHO it makes sense if you can have more than one that there isn't a singular version. Since it was done in 2016, by this point you can probably delete it.

ShimShtein

We also need to add mocked data to

fog-vsphere/lib/fog/vsphere/compute.rb

Line 352 in 4a7f4e7

def self.data

to contain at least one host with NVME controller and a drive that is attached to it.

ShimShtein · 2024-05-22T06:29:56Z

lib/fog/vsphere/models/compute/nvmecontroller.rb

+          self.key ||= 2000
+          self.type ||= "VirtualNVMEController"


Can we extract those into constants:

DEFAULT_KEY = 2000 DEFAULT_TYPE = "VirtualNVMEController" # ... self.key ||= DEFAULT_KEY self.type ||= DEFAULT_TYPE

ShimShtein · 2024-05-22T06:30:37Z

lib/fog/vsphere/models/compute/scsicontroller.rb

@@ -11,6 +11,7 @@ class SCSIController < Fog::Model
        def initialize(attributes = {})
          super
          self.key ||= 1000
+          self.type ||= "VirtualLsiLogicController"


Would be nice to extract to constant too

ShimShtein · 2024-05-22T06:31:44Z

lib/fog/vsphere/models/compute/server.rb

@@ -7,6 +7,7 @@ class Server < Fog::Compute::Server
        extend Fog::Deprecation
        deprecate(:ipaddress, :public_ip_address)
        deprecate(:scsi_controller, :scsi_controllers)
+        deprecate(:nvme_controller, :nvme_controllers)


I suppose we can skip creating this method in the first place. If we don't want people to use it - we can skip creating it.

ShimShtein · 2024-05-22T06:39:00Z

lib/fog/vsphere/models/compute/server.rb

+        end
+
+        def update_controller_key(vol)
+          if !attributes[:scsi_controllers].empty? && !vol[:controller_key]


Personally I like early returns, this way we are clearly defining a precondition for the rest of the function

return if vol[:controller_key]

And then we can avoid the if statement altogether:

attributes[:scsi_controllers].first&.key || 1000

ShimShtein · 2024-05-22T06:46:12Z

lib/fog/vsphere/models/compute/server.rb

        def initialize_volumes
-          if attributes[:volumes] && attributes[:volumes].is_a?(Array)
+          if attributes[:volumes] && attributes[:volumes].is_a?(Array) && !attributes[:volumes].empty?


I think precondition here would be also more readable:

return unless attributes[:volumes].is_a?(Array) && !attributes[:volumes].empty?

We can skip the first condition entirely, since:

[4] pry(main)> nil.is_a?(Array) => false

ShimShtein · 2024-05-22T10:18:47Z

lib/fog/vsphere/models/compute/server.rb

+            attributes[:nvme_controllers].map! do |controller|
+              controller.is_a?(Hash) ? Fog::Vsphere::Compute::NVMEController.new(controller) : controller
+            end
+          elsif attributes[:nvme_controller] && attributes[:nvme_controller].is_a?(Hash)


Since we decided that nvme_controller should be removed, this part will need to be removed also.

ShimShtein · 2024-06-09T11:32:55Z

Not tested, but code-wise looks good

chris1984 · 2024-06-10T13:59:35Z

Starting to test

ekohl

I don't see the Layout/EmptyLineAfterGuardClause enabled here, but I like the style where you leave an empty line after a return statement.

ekohl · 2024-05-31T08:34:10Z

.rubocop.yml

+Metrics/BlockLength:
+  Enabled: false
+
+Style/MutableConstant:


Please keep this cop enabled

ekohl · 2024-06-12T11:41:37Z

.rubocop.yml

+Metrics/ClassLength:
+  Enabled: false
+
+Metrics/BlockLength:
+  Enabled: false


I was more thinking about disabling them in the Ruby files where needed rather than globally.

ekohl · 2024-06-12T11:43:35Z

lib/fog/vsphere/models/compute/server.rb

-                vol.server = self
-                vol
-              end
+          return unless attributes[:volumes].is_a?(Array) && !attributes[:volumes].empty?


I'd leave out the empty check because calling map on an empty array is essentially a noop.

Suggested change

return unless attributes[:volumes].is_a?(Array) && !attributes[:volumes].empty?

return unless attributes[:volumes].is_a?(Array)

ekohl · 2024-06-12T11:44:41Z

lib/fog/vsphere/models/compute/server.rb

+            return if vol[:controller_key]
+            vol[:controller_key] = attributes[:scsi_controllers].first&.key || 1000


A more common pattern is:

Suggested change

return if vol[:controller_key]

vol[:controller_key] = attributes[:scsi_controllers].first&.key || 1000

vol[:controller_key] ||= attributes[:scsi_controllers].first&.key || 1000

ekohl · 2024-06-12T11:51:43Z

lib/fog/vsphere/models/compute/server.rb

+        end
+
+        def initialize_nvme_controllers
+          if attributes[:nvme_controllers] && attributes[:nvme_controllers].is_a?(Array) && !attributes[:nvme_controllers].empty?


This is sufficient:

Suggested change

if attributes[:nvme_controllers] && attributes[:nvme_controllers].is_a?(Array) && !attributes[:nvme_controllers].empty?

if attributes[:nvme_controllers].is_a?(Array)

attributes[:nvme_controllers] must be true for attributes[:nvme_controllers].is_a?(Array) to be true because only false and nil are the only values that evaluate to false. Neither false.is_a?(Array) nor nil.is_a?(Array) would pass, so you can drop it.

The empty check is also redundant, because calling map! on an empty array would be a noop.

ekohl · 2024-06-12T11:57:22Z

.rubocop.yml

+  Enabled: false
+
+Style/MutableConstant:
+  Enabled: false


Please make sure you keep line endings at the end of files.

ekohl · 2024-06-12T11:57:47Z

lib/fog/vsphere/requests/compute/create_vm.rb

@@ -142,6 +142,10 @@ def device_change(attributes)
            devices << scsi_controllers.each_with_index.map { |controller, index| create_controller(controller, index) }
          end

+          if (nvme_controllers = (attributes[:nvme_controllers]))


Suggested change

if (nvme_controllers = (attributes[:nvme_controllers]))

if (nvme_controllers = attributes[:nvme_controllers])

ekohl · 2024-06-12T12:03:31Z

lib/fog/vsphere/requests/compute/create_vm.rb

@@ -255,14 +259,18 @@ def create_controller(controller = nil, index = 0)
            operation: options[:operation],
            device: controller_class.new(key: options[:key] || (1000 + index),
                                         busNumber: options[:bus_id] || index,
-                                         **(options[:type] == RbVmomi::VIM::VirtualAHCIController ? {} : {sharedBus: controller_get_shared_from_options(options)}))
+                                         **(check_type(options[:type]) ? {} : {sharedBus: controller_get_shared_from_options(options)}))


check_type is a really generic name. I'd be tempted to write the class as something like:

def create_controller(controller = nil, index = 0) # ... controller_class = ... device_options = {} unless [RbVmomi::VIM::VirtualAHCIController, RbVmomi::VIM::VirtualNVMEController, "VirtualNVMEController"].include?(options[:type]) device_options[:sharedBus] = controller_get_shared_from_options(options) end { operation: options[:operation], device: controller_class.new(key: options[:key] || (1000 + index), busNumber: options[:bus_id] || index, **device_options) }

And then you can consider the following, but now we're really getting in the code refactoring realm.

device_options = { key: options.fetch(:key, 1000 + index), busNumber: options.fetch(:bus_id, index), }

ekohl · 2024-06-12T13:06:59Z

lib/fog/vsphere/requests/compute/get_vm_first_nvme_controller.rb

+          # require 'pry'
+          # binding.pry
+          ctrl = get_vm_ref(vm_id).config.hardware.device.find { |device| device.is_a?(RbVmomi::VIM::VirtualNVMEController) }
+          raise(Fog::Vsphere::Compute::NotFound) unless ctrl


I'd be explicit what wasn't found

Suggested change

raise(Fog::Vsphere::Compute::NotFound) unless ctrl

raise Fog::Vsphere::Compute::NotFound, "No NVME controller found for #{vm_id}" unless ctrl

ekohl · 2024-06-12T13:07:07Z

lib/fog/vsphere/requests/compute/get_vm_first_nvme_controller.rb

+          # require 'pry'
+          # binding.pry


Suggested change

# require 'pry'

# binding.pry

ready for re-review

ShimShtein · 2024-06-24T09:48:54Z

lib/fog/vsphere/compute.rb

@@ -346,11 +349,11 @@ def is_uuid?(id)
        end
      end

-      class Mock
+      class Mock # rubocop:disable Metrics/ClassLength


You will need to reenable the cop after the class

ShimShtein · 2024-06-24T09:49:34Z

lib/fog/vsphere/compute.rb

        include Shared
        # rubocop:disable Metrics/MethodLength
        def self.data
-          @data ||= Hash.new do |hash, key|
+          @data ||= Hash.new do |hash, key| # rubocop:disable Metrics/BlockLength


You will need to reenable the cop after the block

I think its not required anymore

It's the difference between an inline comment and one that is on its own.

We technically don't need to re-enable it when inline but since we've been using that enable - disable format throughout, I have modified it

ShimShtein · 2024-06-24T10:04:00Z

lib/fog/vsphere/models/compute/server.rb

+            update_controller_key(vol)
+            if vol.is_a?(Hash)
+              service.volumes.new({ server: self }.merge(vol))
+            else
+              vol.server = self
+              vol


If you reorder the calls to:

Suggested change

update_controller_key(vol)

if vol.is_a?(Hash)

service.volumes.new({ server: self }.merge(vol))

else

vol.server = self

vol

if vol.is_a?(Hash)

vol = service.volumes.new({ server: self }.merge(vol))

end

vol.server = self

update_controller_key(vol)

vol

You will be able to remove the case statement in update_controller_key:

def update_controller_key(vol) vol.controller_key ||= attributes[:scsi_controllers].first&.key || 1000 end

because vol will always contain an object.

Interesting. Thanks Shim, updated :)

ShimShtein · 2024-06-24T10:08:26Z

lib/fog/vsphere/requests/compute/get_vm_first_nvme_controller.rb

+        end
+      end
+      class Mock
+        def get_vm_first_nvme_controller(vm_id); end


The mock class returns nil, while the real method will throw an exception and never return nil.
I think it would be nice to have the behavior be equal

We are not doing that for the other(scsi and sata) controllers, I dont think it's necessary to do that as it's the mock class.

ShimShtein · 2024-06-24T10:11:29Z

lib/fog/vsphere/requests/compute/vm_clone.rb

@@ -887,7 +886,6 @@ def relocation_volume_backing(volume)
        end
      end

-      # rubocop:enable Metrics/ClassLength


I think you should reenable the class length metric after the class definition.

chris1984 · 2024-06-24T13:34:25Z

Starting to test again, @ekohl have all your concerns been addressed? If so will merge and get a release out pending all testing works again with the latest pushes.

ekohl · 2024-06-24T14:56:53Z

lib/fog/vsphere/requests/compute/create_vm.rb

@@ -255,7 +263,7 @@ def create_controller(controller = nil, index = 0)
            operation: options[:operation],
            device: controller_class.new(key: options[:key] || (1000 + index),
                                         busNumber: options[:bus_id] || index,
-                                         **(options[:type] == RbVmomi::VIM::VirtualAHCIController ? {} : {sharedBus: controller_get_shared_from_options(options)}))
+            **device_options)


The indenting looks off here

No, it's fine I had cross checked

This looks off if I look at the file, could it be your editor?

fog-vsphere/lib/fog/vsphere/requests/compute/create_vm.rb

Line 266 in 9b71bef

**device_options)

Updated with a new indentation. Either way, was accepted by rubocop and the general editor format

chris1984 · 2024-06-26T13:19:02Z

Looks fine to me @ekohl any other thoughts before I do a final round of testing.

ekohl · 2024-06-27T10:36:44Z

.rubocop.yml

@@ -42,3 +42,7 @@ Style/RescueModifier:

 Metrics/MethodLength:
  Max: 45
+
+Style/MutableConstant:


I'd be hesitant to disable this. Why is it needed?

used the freeze method for the string constants

chris1984 · 2024-07-02T13:30:00Z

@ekohl any other comments or is this good to go, tested it and it works with the latest commit and subhash did as well.

ekohl · 2024-07-04T15:40:31Z

@chris1984 I still see open conversations when I look on the changed files tab. I'd expect those to either be fixed or closed with an explanation why it won't be fixed.

girijaasoni · 2024-07-08T06:31:36Z

@chris1984 I still see open conversations when I look on the changed files tab. I'd expect those to either be fixed or closed with an explanation why it won't be fixed.

Thanks @ekohl , updated them.

chris1984 · 2024-07-10T15:16:34Z

Looks fine now, just one concern:

I tested this again and I am getting Failed to create a compute VMware8 (VMware) instance ron-doria.satellite.lab.eng.rdu2.redhat.com: InvalidController: The device '1' is referring to a nonexisting controller '1,000'.

Since it keeps happening between my env and qe's I wonder if customers will start to hit this too. Is there a way we can get that controller id for all envs since you make a fix and it works on mine, but errors's on qe's vmware then you fix and it works on theirs but not mine. I am worried customer will hit that same error

chris1984

ACK, tested it on 6/7/8 and it works fine as well as on QE's VMware 8:

girijaasoni mentioned this pull request May 15, 2024

Fixes #34839 - Add support for VMware NVME Controllers theforeman/foreman#10168

Merged

girijaasoni force-pushed the nvme-controllers branch from 2c9aa8e to e425c52 Compare May 16, 2024 10:07

ekohl reviewed May 17, 2024

View reviewed changes

ShimShtein suggested changes May 22, 2024

View reviewed changes

girijaasoni force-pushed the nvme-controllers branch 5 times, most recently from 0ca767a to e8469e2 Compare May 31, 2024 14:07

girijaasoni mentioned this pull request Jun 3, 2024

Fixes #37689 - Add NVME controllers to hammer help theforeman/hammer-cli-foreman#628

Merged

ekohl suggested changes Jun 12, 2024

View reviewed changes

girijaasoni force-pushed the nvme-controllers branch 2 times, most recently from 35135ee to 26303a4 Compare June 24, 2024 06:41

ShimShtein suggested changes Jun 24, 2024

View reviewed changes

girijaasoni force-pushed the nvme-controllers branch from 26303a4 to c8224bf Compare June 24, 2024 11:31

ekohl reviewed Jun 24, 2024

View reviewed changes

girijaasoni force-pushed the nvme-controllers branch 2 times, most recently from 9b71bef to 5298015 Compare June 26, 2024 11:13

ekohl reviewed Jun 27, 2024

View reviewed changes

girijaasoni force-pushed the nvme-controllers branch from 5298015 to 1ea57bc Compare June 27, 2024 10:56

girijaasoni requested a review from ekohl July 2, 2024 13:05

Add support for NVME Controllers

5ba744d

girijaasoni force-pushed the nvme-controllers branch from 1ea57bc to 5ba744d Compare July 8, 2024 06:29

girijaasoni requested a review from chris1984 July 10, 2024 10:11

chris1984 approved these changes Jul 11, 2024

View reviewed changes

chris1984 merged commit e5eed8f into fog:master Jul 11, 2024
3 of 5 checks passed

	return unless attributes[:volumes].is_a?(Array) && !attributes[:volumes].empty?
	return unless attributes[:volumes].is_a?(Array)

		return if vol[:controller_key]
		vol[:controller_key] = attributes[:scsi_controllers].first&.key \|\| 1000

	return if vol[:controller_key]
	vol[:controller_key] = attributes[:scsi_controllers].first&.key \|\| 1000
	vol[:controller_key] \|\|= attributes[:scsi_controllers].first&.key \|\| 1000

	if attributes[:nvme_controllers] && attributes[:nvme_controllers].is_a?(Array) && !attributes[:nvme_controllers].empty?
	if attributes[:nvme_controllers].is_a?(Array)

	if (nvme_controllers = (attributes[:nvme_controllers]))
	if (nvme_controllers = attributes[:nvme_controllers])

	raise(Fog::Vsphere::Compute::NotFound) unless ctrl
	raise Fog::Vsphere::Compute::NotFound, "No NVME controller found for #{vm_id}" unless ctrl

Add support for NVME Controllers #300

Add support for NVME Controllers #300

Conversation

girijaasoni commented May 15, 2024 • edited Loading

ekohl 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

ShimShtein 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

ShimShtein commented Jun 9, 2024

chris1984 commented Jun 10, 2024

ekohl 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

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

chris1984 commented Jun 24, 2024

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

chris1984 commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris1984 commented Jul 2, 2024

ekohl commented Jul 4, 2024

girijaasoni commented Jul 8, 2024

chris1984 commented Jul 10, 2024

chris1984 left a comment

Choose a reason for hiding this comment

girijaasoni commented May 15, 2024 •

edited

Loading