Fixes #34839 - Add support for VMware NVME Controllers #10168

girijaasoni · 2024-05-15T00:51:32Z

blocked by PR
hammer doc PR

I put together a document for better understanding. Please feel free to go through it and provide suggestions/improvisations.

Ready for testing :D

Test cases need the fog-vsphere PR merge. They pass on my local and we can trigger them again after the merge of fog-vpshere PR!

ShimShtein · 2024-05-22T10:41:32Z

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

+ attrs["nvme_controllers"] = ctrls_and_vol[:controllers]&.select { |controller| controller[:type].include?("VirtualNVMEController") }
+ attrs["scsi_controllers"] = ctrls_and_vol[:controllers]&.select { |controller| !controller[:type].include?("VirtualNVMEController") }


You can use #split method to divide the controllers part

I have used #partition instead, wasn't able to understand the use case #split

ShimShtein · 2024-05-22T10:48:24Z

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

@@ -484,7 +485,7 @@ def parse_args(args)

 # see #26402 - consume scsi_controller_type from hammer as a default scsi type
 scsi_type = args.delete(:scsi_controller_type)


We will need to create a generic option for Hammer too here: instead of scsi_controller_type it would become controller_type. Plus we will need to deprecate the older scsi_controller_type

scsi_controller_type has been removed from compute attributes in this commit Its best we remove this part all together as we use scsi_controller attribute to pass the type and the key

ShimShtein · 2024-05-22T10:51:12Z

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

 def new_vm(args = {})
 args = parse_args args
+ args = args.deep_symbolize_keys
+ args[:scsi_controllers] = [] if !args.key?(:scsi_controllers) && !args[:volumes].empty? && !unassigned_volumes?(args[:volumes])


A comment explaining why do you need to set scsi_controllers to an empty array would be nice here.

ShimShtein · 2024-05-22T10:56:27Z

webpack/assets/javascripts/react_app/redux/reducers/hosts/storage/vmware.js

@@ -26,7 +26,18 @@ const initialState = Immutable({
 volumes: [],
 });

-const availableControllerKeys = [1000, 1001, 1002, 1003, 1004];
+const availableControllerKeys = [
+ 1000,


Can you please give me a bit more details about this array and the meaning of the items in it?

This is to assign the keys to scsi or nvme controllers. We can have max of 4 controllers each so the total number of available keys are 8.

When I was in university my professor would reduce your grade if you used magic values. Anything other than 0, 1 or 2 needed to be a constant that described what the magic value actually was.

In Foreman that rule isn't strictly applied, but another way to write this would be:

Array.from({length: 8}, (value, index) => 1000 + index);

I'm not sure if it's that much clearer, but as I read the code it's a list of 8 numbers where the base number is 1000.

Having said that, how do you prevent a user from assigning 8 nvme controllers or 8 scsi controllers?

That is handled by Rbvmomi. We dont place any restrictions from the UI or the api or hammer

ekohl · 2024-05-22T16:29:27Z

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

Since controller is ambiguos, I'd suggest to be a bit more explicit and name it something like normalize_vmware_storage_controller_attributes.rb

ekohl · 2024-05-22T16:30:35Z

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

@@ -192,12 +192,13 @@ def nictypes
 }
 end

- def scsi_controller_types
+ def controller_types


storage_controller_types?

ekohl

Test failures look related.

ekohl · 2024-06-03T12:02:20Z

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

+ volumes = {}
+ ctrls_and_vol[:volumes].each_with_index do |vol, index|
+ volumes[index.to_s] = vol
+ end


Suggested change

volumes = {}

ctrls_and_vol[:volumes].each_with_index do |vol, index|

volumes[index.to_s] = vol

end

volumes = ctrls_and_vol[:volumes].each_with_index.to_h { |vol, index| [index.to_s, vol] }

ekohl · 2024-06-03T12:04:47Z

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

+ normalized['nvme_controllers'] = nvme_controllers.map.with_index do |ctrl, idx|
+ [idx.to_s, ctrl]
+ end.to_h


Suggested change

normalized['nvme_controllers'] = nvme_controllers.map.with_index do |ctrl, idx|

[idx.to_s, ctrl]

end.to_h

normalized['nvme_controllers'] = nvme_controllers.with_index.to_h do |ctrl, idx|

[idx.to_s, ctrl]

end

girijaasoni · 2024-06-03T12:11:00Z

Test failures look related.

Test cases are related to changes in the fog-vpshere. They all pass on my local and we can re-trigger them once the fog-vsphere changes are in.

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

Test cases are related to changes in the fog-vpshere. They all pass on my local and we can re-trigger them once the fog-vsphere changes are in.

You can change this line here:

foreman/bundler.d/vmware.rb

Line 2 in 89a3f5f

gem 'fog-vsphere', '>= 3.6.4', '< 4.0'

If you point that to your git branch (using https://bundler.io/guides/git.html) then you can run CI here. Once a release of fog-vsphere it out, you can change the line to require that specific release which includes the feature. That way our packaging will either fail or pick it up properly. We have some automation in that area.

ekohl

I realize some of the comments would have also applied to the existing code you're moving around, but whenever you add code it's also a good chance to revisit the code around it.

ekohl · 2024-06-03T14:00:40Z

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

+ private
+
+ def normalize_vmware_storage_controller_attributes(attrs)
+ ctrls_and_vol = JSON.parse(attrs["controllers"]).


You can delete here and drop line 13.

Suggested change

ctrls_and_vol = JSON.parse(attrs["controllers"]).

ctrls_and_vol = JSON.parse(attrs.delete("controllers")).

we need to delete attrs[controllers] after we extract nvme and scsi controllers from them, Hence, the deleting part is done on line 13

I don't see it used. You use ctrls_and_vol[:controllers] later on, but you don't depend on attrs["controllers"].

ekohl · 2024-06-03T14:03:11Z

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

+ 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.to_h { |vol, index| [index.to_s, vol] }


Perhaps it makes sense to combine line 13 & this one:

Suggested change

volumes = ctrls_and_vol[:volumes].each_with_index.to_h { |vol, index| [index.to_s, vol] }

attrs["volumes_attributes"] = ctrls_and_vol[:volumes].each_with_index.to_h { |vol, index| [index.to_s, vol] }

ekohl · 2024-06-03T14:03:28Z

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

@@ -638,7 +642,12 @@ def new_interface(attr = { })
 end

 def new_volume(attr = { })
- client.volumes.new attr.merge(:size_gb => 10)
+ {
+ :thin => true,


Without looking, don't we have some option to control whether it's thin or thick?

You're also completely ignoring the attr now. Is that intended as well?

No, thin is just the default. We can change it as per convenience.

Ah I didn't notice attr being unused. Thanks!!

ekohl · 2024-06-03T14:05:33Z

bundler.d/vmware.rb

@@ -1,3 +1,3 @@
 group :vmware do
- gem 'fog-vsphere', '>= 3.6.4', '< 4.0'
+ gem 'fog-vsphere', git: 'https://github.com/girijaasoni/fog-vsphere.git', branch: 'nvme-controllers'


This is fine and works (so no need to change it), but if you're lazy (like me) you can also use:

Suggested change

gem 'fog-vsphere', git: 'https://github.com/girijaasoni/fog-vsphere.git', branch: 'nvme-controllers'

gem 'fog-vsphere', github: 'girijaasoni/fog-vsphere', branch: 'nvme-controllers'

Having to type less is especially useful if you make quick local modifications.

ekohl · 2024-06-04T15:22:36Z

app/helpers/compute_resources_vms_helper.rb

 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


Again, already a problem before but I think this whole method can be implemented as:

compute_resource.storage_controller_types.to_h { |type| [type[:key], type[:title]] }

girijaasoni requested a review from a team as a code owner May 15, 2024 00:51

pr-processor bot added the Not yet reviewed label May 15, 2024

github-actions bot added the UI label May 15, 2024

girijaasoni force-pushed the nvme_controllers branch from 2b0d314 to 15201f1 Compare May 15, 2024 00:55

girijaasoni marked this pull request as draft May 15, 2024 01:53

girijaasoni force-pushed the nvme_controllers branch from 15201f1 to 7b1d6ae Compare May 16, 2024 10:10

girijaasoni marked this pull request as ready for review May 16, 2024 10:14

girijaasoni force-pushed the nvme_controllers branch from 7b1d6ae to 267a5f4 Compare May 16, 2024 10:53

ekohl changed the title ~~Fixes #34839 - Add support for NVME Controllers~~ Fixes #34839 - Add support for VMware NVME Controllers May 17, 2024

girijaasoni mentioned this pull request May 17, 2024

Add support for NVME Controllers fog/fog-vsphere#300

Open

stejskalleos assigned stejskalleos and ShimShtein May 22, 2024

ShimShtein requested changes May 22, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels May 22, 2024

ekohl reviewed May 22, 2024

View reviewed changes

girijaasoni force-pushed the nvme_controllers branch from 267a5f4 to 28c3277 Compare May 30, 2024 13:47

pr-processor bot added Needs re-review and removed Waiting on contributor labels May 30, 2024

girijaasoni force-pushed the nvme_controllers branch 2 times, most recently from d4dfa53 to d4c33a1 Compare May 31, 2024 06:58

girijaasoni mentioned this pull request Jun 3, 2024

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

Open

ekohl reviewed Jun 3, 2024

View reviewed changes

girijaasoni force-pushed the nvme_controllers branch 5 times, most recently from 4baa998 to 2eb9c33 Compare June 3, 2024 13:46

Fixes #34839 - Add support for NVME Controllers

3b292ae

girijaasoni force-pushed the nvme_controllers branch from 2eb9c33 to 3b292ae Compare June 3, 2024 14:01

ekohl reviewed Jun 3, 2024

View reviewed changes

ekohl reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #34839 - Add support for VMware NVME Controllers #10168

Fixes #34839 - Add support for VMware NVME Controllers #10168

girijaasoni commented May 15, 2024 •

edited

ShimShtein May 22, 2024

girijaasoni Jun 3, 2024

ShimShtein May 22, 2024

girijaasoni May 30, 2024 •

edited

ShimShtein May 22, 2024

ShimShtein May 22, 2024

girijaasoni May 30, 2024

ekohl May 30, 2024

girijaasoni May 30, 2024

ekohl May 22, 2024

ekohl May 22, 2024

ekohl left a comment

ekohl Jun 3, 2024

ekohl Jun 3, 2024

girijaasoni commented Jun 3, 2024

ekohl commented Jun 3, 2024

ekohl left a comment

ekohl Jun 3, 2024

girijaasoni Jun 4, 2024

ekohl Jun 4, 2024

ekohl Jun 3, 2024

ekohl Jun 3, 2024

girijaasoni Jun 4, 2024

girijaasoni Jun 4, 2024

ekohl Jun 3, 2024

ekohl Jun 4, 2024

		attrs["nvme_controllers"] = ctrls_and_vol[:controllers]&.select { \|controller\| controller[:type].include?("VirtualNVMEController") }
		attrs["scsi_controllers"] = ctrls_and_vol[:controllers]&.select { \|controller\| !controller[:type].include?("VirtualNVMEController") }

		@@ -484,7 +485,7 @@ def parse_args(args)

		# see #26402 - consume scsi_controller_type from hammer as a default scsi type
		scsi_type = args.delete(:scsi_controller_type)

	ctrls_and_vol = JSON.parse(attrs["controllers"]).
	ctrls_and_vol = JSON.parse(attrs.delete("controllers")).

	volumes = ctrls_and_vol[:volumes].each_with_index.to_h { \|vol, index\| [index.to_s, vol] }
	attrs["volumes_attributes"] = ctrls_and_vol[:volumes].each_with_index.to_h { \|vol, index\| [index.to_s, vol] }

	gem 'fog-vsphere', git: 'https://github.com/girijaasoni/fog-vsphere.git', branch: 'nvme-controllers'
	gem 'fog-vsphere', github: 'girijaasoni/fog-vsphere', branch: 'nvme-controllers'

Fixes #34839 - Add support for VMware NVME Controllers #10168

Are you sure you want to change the base?

Fixes #34839 - Add support for VMware NVME Controllers #10168

Conversation

girijaasoni commented May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni May 30, 2024 • edited

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

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

girijaasoni commented Jun 3, 2024

ekohl commented Jun 3, 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

girijaasoni commented May 15, 2024 •

edited

girijaasoni May 30, 2024 •

edited