Some improvements on vmware_guest_disk module

[Tomorrow9]

SUMMARY

ISSUE TYPE

• Feature Idea

COMPONENT NAME

vmware_guest_disk

ADDITIONAL INFORMATION

In current implementation, the new disk controllers and new disks are handled/configured separately as below, disk controllers will be added first, then new disks will be added. (There is error when add them at the same time when I tried to implement this way before.)

if ctl_changed:
    self.reconfigure_vm(self.config_spec, 'Disk Controller')      
    self.config_spec = vim.vm.ConfigSpec()
    self.config_spec.deviceChange = []

Normally there is no issue about this, but when we need to add a controller and a disk at the same time, e.g., NVMe controller and disk need to be added at the same time then guest OS can recognize this new disk on some ESXi host versions or some Linux guest OS. So I wonder if we can also make some changes on this module like "vmware_guest_network" module does, add one controller and one disk at the same time in one task, if users want to add more than one disks, execute this the task several times.

[Tomorrow9]

So that we can cover all the scenarios:

1. use vmware_guest_controller to add new controller,
2. use vmware_guest_disk to add new disk to existing disk controller added in 1.,
3. use vmware_guest_disk to add new disk controller and new disk at the same time.

Usage of vmware_guest_disk can be like this:

community.vmware.vmware_guest_disk:
   hostname: "{{ vcenter_hostname }}"
   username: "{{ vcenter_username }}"
   password: "{{ vcenter_password }}"
   datacenter: "{{ datacenter_name }}"
   validate_certs: no
   name: VM_test
   controller_type: nvme
   controller_number: 1
   disk:
      - size_mb: 256
         type: thin
         datastore: datacluster0
         unit_number: 1
         state: present
      - size_gb: 1
         unit_number: 2
         state: present

Or only one disk controller with one disk one time:

community.vmware.vmware_guest_disk:
   hostname: "{{ vcenter_hostname }}"
   username: "{{ vcenter_username }}"
   password: "{{ vcenter_password }}"
   datacenter: "{{ datacenter_name }}"
   validate_certs: no
   name: VM_test
   controller_type: nvme
   controller_number: 1
   size_mb: 256
   type: thin
   datastore: datacluster0
   unit_number: 1
   state: present

[Tomorrow9]

@Akasurde @goneri @mariolenz @sky-joker what do you think about this proposal? Thanks.

[mariolenz]

Normally there is no issue about this, but when we need to add a controller and a disk at the same time, e.g., NVMe controller and disk need to be added at the same time then guest OS can recognize this new disk on some ESXi host versions or some Linux guest OS. So I wonder if we can also make some changes on this module like "vmware_guest_network" module does, add one controller and one disk at the same time in one task, if users want to add more than one disks, execute this the task several times.

To tell you the truth, I'm not really sure... if I understand your proposal correct, this would fix one special case: If you add only one disk. It sounds like we'll run into the problem that Linux doesn't recognize disks 2+ if users want to add more than one.

Actually, I think it would be more important to move the handling of storage controllers and disks to a library (plugins/module_utils/vmware.py?) and use this in vmware_guest and vmware_guest_disk. We have some duplicate code there. Now that I think about this, maybe we should create a new class ansible_collections.community.vmware.plugins.module_utils.vmware_guest that inherits from ansible_collections.community.vmware.plugins.module_utils.vmware and implements the stuff needed by more than one guest related module.

Btw: Do you run into the same issue if you use vmware_guest to add a new controller and a new disk at the same time?

Thanks for starting this discussion! This might get really interesting :-)

[Tomorrow9]

To tell you the truth, I'm not really sure... if I understand your proposal correct, this would fix one special case: If you add only one disk. It sounds like we'll run into the problem that Linux doesn't recognize disks 2+ if users want to add more than one.

Yeah, you are right about this, it only work for the first time adding the controller and disk at the same time, but when add disk to the exiting controller, there is still the problem.

Actually, I think it would be more important to move the handling of storage controllers and disks to a library (plugins/module_utils/vmware.py?) and use this in vmware_guest and vmware_guest_disk. We have some duplicate code there. Now that I think about this, maybe we should create a new class ansible_collections.community.vmware.plugins.module_utils.vmware_guest that inherits from ansible_collections.community.vmware.plugins.module_utils.vmware and implements the stuff needed by more than one guest related module.

This is really a good suggestion, I think this can reduce the duplicated code in many modules and easy for adding new supported parameters.

Btw: Do you run into the same issue if you use vmware_guest to add a new controller and a new disk at the same time?

Yeah, I'll use this module to see if can fulfill our requirement. Thanks.

[Tomorrow9]

Maybe we can start with this new feature request in #853, since vmware_guest and vmware_guest_disk or maybe a new module for NVDIMM device are all required to add this.

[mariolenz]

Maybe we can start with this new feature request in #853, since vmware_guest and vmware_guest_disk or maybe a new module for NVDIMM device are all required to add this.

Personally, I think we should move

community.vmware/plugins/modules/vmware_guest.py

Line 1011 in 99eef38

class PyVmomiDeviceHelper(object):

to module_utils and, as a second step, make vmware_guest_disk use this. Then we can try to implement NVDIMM devices there. This way, both vmware_guest and vmware_guest_disk could make use of NVDIMM devices.

You see, I think we've build up a lot of technical debt by implementing more and more features. But we're not working on paying this dept back.

But that's just my opinion. Let's see what @Akasurde, @goneri, and @sky-joker think about this.

[Tomorrow9]

Since PyVmomiDeviceHelper has been moved to module_utils, I'll close this issue.