Fixes #34839 - Add support for VMware NVME Controllers #10168
Conversation
girijaasoni
force-pushed
the
nvme_controllers
branch
from
2b0d314
to
15201f1
Compare
girijaasoni
force-pushed
the
nvme_controllers
branch
from
15201f1
to
7b1d6ae
Compare
girijaasoni
force-pushed
the
nvme_controllers
branch
from
7b1d6ae
to
267a5f4
Compare
ekohl
changed the title
| 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") } |
There was a problem hiding this comment.
You can use #split method to divide the controllers part
| @@ -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) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| 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]) |
There was a problem hiding this comment.
A comment explaining why do you need to set scsi_controllers to an empty array would be nice here.
| @@ -26,7 +26,18 @@ const initialState = Immutable({ | |||
| volumes: [], | |||
| }); | |||
|
|
|||
| const availableControllerKeys = [1000, 1001, 1002, 1003, 1004]; | |||
| const availableControllerKeys = [ | |||
| 1000, | |||
There was a problem hiding this comment.
Can you please give me a bit more details about this array and the meaning of the items in it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That is handled by Rbvmomi. We dont place any restrictions from the UI or the api or hammer
There was a problem hiding this comment.
Since controller is ambiguos, I'd suggest to be a bit more explicit and name it something like normalize_vmware_storage_controller_attributes.rb
| @@ -192,12 +192,13 @@ def nictypes | |||
| } | |||
| end | |||
|
|
|||
| def scsi_controller_types | |||
| def controller_types | |||
girijaasoni
force-pushed
the
nvme_controllers
branch
from
267a5f4
to
28c3277
Compare
girijaasoni
force-pushed
the
nvme_controllers
branch
from
28c3277
to
d4dfa53
Compare
girijaasoni
force-pushed
the
nvme_controllers
branch
from
d4dfa53
to
d4c33a1
Compare
blocked by PR
Described better in the doc
Ready for testing :D