Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promote vmware guest #4

Closed

Conversation

machacekondra
Copy link
Collaborator

No description provided.

goneri and others added 30 commits March 8, 2024 10:41
Ensure the idempotency of the module if we try to delete a NIC two times
in a row.

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@8bd5dbc
If new disk filename is not specified, vmware esxi will set new disk
filename with a not ever used index, so even recreate new disk with
same controller number and same unit number, filename existence
failure would not occur.

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@592db1a
Handle list items in vSphere schema while handling facts in vmware_guest_info

Fixes: ansible-collections/community.vmware#33

Signed-off-by: Abhijeet Kasurde <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@6a4fe4f
…rk (ansible-collections#29)

Add check mode, diff and some new functionality to vmware_guest_network

Reviewed-by: Gonéri Le Bouder <[email protected]>
             https://github.com/goneri

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@4c9869c
dericcrago and others added 19 commits March 8, 2024 10:41
vmware_guest - set custom attributes on VM create

SUMMARY
Fixes issue where custom attributes were not getting set on VM creation.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

plugins/modules/vmware_guest.py

ADDITIONAL INFORMATION

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@ab37851
Update community.vmware.vmware_guest_module.rst

Replace missing end of sentence
SUMMARY
Previous update to the vmware_guest_module cut off end of sentence, losing context and version info.
ISSUE TYPE

Docs Pull Request

COMPONENT NAME
vmware_guest_module
ADDITIONAL INFORMATION
https://docs.ansible.com/ansible/2.9/modules/vmware_guest_module.html
Compare the 'folder' parameter between versions 2.9 and the latest.

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@09ccfdf
CI: Remove unnecessary prepare step

I think this task is not really necessary:

      community.vmware/tests/integration/targets/prepare_vmware_tests/tasks/setup_datastore.yml

        Lines 30 to 32
      in
      f476a07

           - vmware_host_scanhba:

               refresh_storage: true

               cluster_name: '{{ ccr1 }}'

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@f437f0b
Do not fail when state is absent and disk is not found

SUMMARY
Fixes #1765. Do not faill when state is absent and disk is not present
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
vmware_guest_disk
ADDITIONAL INFORMATION

PLAY [localhost] ***********************************************************************************************************************************************************************************************************************

TASK [Remove VM disk] ******************************************************************************************************************************************************************************************************************

ok: [localhost]

PLAY RECAP *****************************************************************************************************************************************************************************************************************************

localhost                  : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@7658cbe
Also look at NFS mounted datastores, not just VMFS

SUMMARY

VMware allows datastores to be NFS hosted. Currently the autoselect_datastore logic only considers VMFS filesystems. NFS mounted filesystems show up as "NFS", so adjusting logic to also check and consider those filesystems.
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
Make autoselect_datastore consider NFS mounted filesystems.
ADDITIONAL INFORMATION
Code change does an "or" test on the volume type looking for things that are "NFS" volume types.
Validation requires setting up NFS datastores in your ESXi environment, then calling vmware_guest as below:

  community.vmware.vmware_guest:
    hostname: "{{ vmware_vcenter }}"
    username: "{{ vmware_account_name }}"
    password: "{{ vmware_password }}"
    validate_certs: False
    name: "{{ vmware_create_vm_name }}"
    datacenter: "{{ vmware_datacenter }}"
    cluster: "{{ vmware_cluster }}"
    folder: "{{ vmware_folder }}"
    hardware:
      num_cpus: "{{ vmware_create_vm_num_cpus }}"
      memory_mb: "{{ vmware_create_vm_memory_mb }}"
    disk:
      - size_gb: "{{ vmware_create_vm_disk_size_gb }}"
        type: "thick"
        datastore: "{{ vmware_create_vm_disk_datastore }}"
        autoselect_datastore: "{{ vmware_create_vm_disk_autoselect_datastore }}"
    networks:
      - connected: yes
        start_connected: yes
        name: "{{ vmware_create_vm_networks_name }}"
    cdrom:
      - type: "iso"
        controller_number: 0
        unit_number: 0
        iso_path: "{{ vmware_create_vm_cdrom_iso_path }}"
  delegate_to: 127.0.0.1

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@255627d
Implement encryption configuration for vm vmotion and ft

SUMMARY
Fixes #1069
Implements the  ability to configured vm encryption settings for vmotion or fault tolerance

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
vmware_guest
ADDITIONAL INFORMATION
Fulfill  requirements of #1069

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@69d0ce7
Fix typo in Docs for vmware_guest module

SUMMARY

Fix typos

ISSUE TYPE

Docs Pull Request

COMPONENT NAME

vmware_guest

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@1a88289
Add snapshot_id option to vmware_guest_snapshot module

SUMMARY
This PR is to add snapshot_id option to the module.
fixes: #1844
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
changelogs/fragments/1847_vmware_guest_snapshot.yml
docs/community.vmware.vmware_guest_snapshot_module.rst
plugins/modules/vmware_guest_snapshot.py
tests/integration/targets/vmware_guest_snapshot/tasks/main.yml
ADDITIONAL INFORMATION
I will add the integration test in a few days.
Done

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@f94611b
vmware_guest: Remove CD-ROM configuration as dict

SUMMARY
Fixes #1472
Removed specifying CDROM configuration as a dict, instead use a list.
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
vmware_guest
ADDITIONAL INFORMATION
#317

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@136556d
Implement semantic markup (4)

SUMMARY
Implementing semantic markup part 4.
ISSUE TYPE

Docs Pull Request

COMPONENT NAME
vmware_drs_group
vmware_drs_group_info
vmware_drs_group_manager
vmware_drs_rule_info
vmware_dvs_portgroup
vmware_dvs_portgroup_find
vmware_dvswitch
vmware_dvswitch_info
vmware_dvswitch_lacp
vmware_dvswitch_nioc
vmware_dvswitch_pvlans
vmware_evc_mode
vmware_export_ovf
vmware_folder_info
vmware_guest
vmware_guest_boot_info
vmware_guest_boot_manager
vmware_guest_controller
vmware_guest_cross_vc_clone
vmware_guest_custom_attribute_defs
ADDITIONAL INFORMATION
#1771

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@4491bf0
Implement semantic markup (5)

SUMMARY
Implementing semantic markup part 5.
ISSUE TYPE

Docs Pull Request

COMPONENT NAME
vmware_guest_custom_attributes
vmware_guest_disk
vmware_guest_disk_info
vmware_guest_file_operation
vmware_guest_find
vmware_guest_info
vmware_guest_instant_clone
vmware_guest_move
vmware_guest_network
vmware_guest_powerstate
vmware_guest_register_operation
vmware_guest_screenshot
vmware_guest_sendkey
vmware_guest_serial_port
vmware_guest_snapshot
vmware_guest_snapshot_info
vmware_guest_storage_policy
vmware_guest_tools_info
vmware_guest_tools_upgrade
vmware_guest_tools_wait
vmware_guest_tpm
vmware_guest_vgpu
vmware_guest_vgpu_info
vmware_guest_video
ADDITIONAL INFORMATION
#1771

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@78d6bc1
Fix integration tests

SUMMARY
Fix integration tests.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
vmware_about_info
vmware_category
vmware_dvs_portgroup
vmware_folder_info
vmware_guest_controller
vmware_guest_network
ADDITIONAL INFORMATION
#1937
#1938

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@515b149
Add IPv6 support for VM network interfaces

SUMMARY
Add IPv6 support for network interfaces in vmware_guest
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
vmware_guest
ADDITIONAL INFORMATION
The following new parameters are supported in networks section:
typev6: optional, either static or dhcp (defaults to static, if ipv6 is given)
ipv6: IPv6 address
netmaskv6: Length of subnet mask (e.g. 64 for a /64 IPv6 address)
gatewayv6: IPv6 address of default gateway
Example of Ansible inventory file snippet:
      hosts:
        testvm:
          name: "testvm"
          network_interfaces:
          - name: "test"
            label: "ens192"
            start_connected: True
            connected: True
            ipv6: "fec0:0:0:1::10"
            netmaskv6: "64"
            gatewayv6: "fec0:0:0:1::1"

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@ccb14cc
…1848)

Fix failure of vm reconfiguration with enabled virt_based_security

SUMMARY
VM reconfiguration is currently broken when virt_based_security is configured due to checking for vbsEnabled on the wrong object.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
vmware_guest

Reviewed-by: Mario Lenz <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@6bde749
…ve (#2020)

Document that all parameters and VMware object names are case sensitive

SUMMARY
Fixes #2019
Document that all parameters and VMware object names are case sensitive.
ISSUE TYPE

Docs Pull Request

COMPONENT NAME
lots

Reviewed-by: Alexander Nikitin <[email protected]>

This commit was initially merged in https://github.com/ansible-collections/community.vmware
See: ansible-collections/community.vmware@b6080ba
@mariolenz
Copy link
Collaborator

@machacekondra Are you really sure you want to have vmware_guest in this collection? As it looks now? Not to put too fine a point on this but IMHO the module tries to do too much, is far too complex (I always think about it as a god module) and not maintainable. People use and like it the way it is, but I think it's pretty problematic.

If you really want to have it, I'm fine with this. If there's one module I'd like to get rid of, it's this one. For the reasons I've stated above. But think twice about it. It's your funeral ;-)

@machacekondra
Copy link
Collaborator Author

machacekondra commented Apr 16, 2024

@machacekondra Are you really sure you want to have vmware_guest in this collection? As it looks now? Not to put too fine a point on this but IMHO the module tries to do too much, is far too complex (I always think about it as a god module) and not maintainable. People use and like it the way it is, but I think it's pretty problematic.

If you really want to have it, I'm fine with this. If there's one module I'd like to get rid of, it's this one. For the reasons I've stated above. But think twice about it. It's your funeral ;-)

I beleive we want to have it, but of course I agree it needs refactoring. How would you propose to make this module more maintanable? Create multiple modules out of it, or just refactor the code and keep the functinality? I beleive our end goal is to keep the interface of the modules, so users just rename the namespace and keep the rest of parameters as is, so we keep the migration path for them as simple as possible.

@mariolenz
Copy link
Collaborator

I beleive we want to have it

Yes, I thought so. I don't have any data, but I guess it's a module that a lot of people use. Maybe even the most used module... but this is just a guess.

but of course I agree it needs refactoring. How would you propose to make this module more maintanable? Create multiple modules out of it, or just refactor the code and keep the functinality?

If I had an idea how to fix this, I'd have already started on working on implementing it in c.vmware 😉

Some ideas:

  1. Drop support for adding a lot of devices when creating a VM. For example, disks. Make people create a VM and use vmware_guest_controller and vmware_guest_disk in following steps.
  2. Drop some other configuration an make people use other modules to achieve this. For example, drop boot_firmware and secure_boot; people can use vmware_guest_boot_manager in a second step to configure this.
  3. Move cloning a VM / template to another module, or maybe two other modules.

It would be a lot of work to figure out what to drop and what to keep, and I'm pretty sure those changes would be extremely unpopular. But I think it would be the only way forward.

I beleive our end goal is to keep the interface of the modules, so users just rename the namespace and keep the rest of parameters as is, so we keep the migration path for them as simple as possible.

I don't think you can have both: A module that's somewhat easy to maintain and keeping it as-is. Refactoring won't help you there. The module just tries to do too many things at once. My personal opinion: Drop functionality and make users angry, or keep functionality and live with a module far too complex. I might be wrong, but at the moment that's how I see things.

@machacekondra
Copy link
Collaborator Author

machacekondra commented Apr 16, 2024

If I had an idea how to fix this, I'd have already started on working on implementing it in c.vmware 😉

Some ideas:

1. Drop support for adding _a lot_ of devices when creating a VM. For example, disks. Make people create a VM and use `vmware_guest_controller` and `vmware_guest_disk` in following steps.

2. Drop some other configuration an make people use other modules to achieve this. For example, drop `boot_firmware` and `secure_boot`; people can use `vmware_guest_boot_manager` in a second step to configure this.

3. Move cloning a VM / template to another module, or maybe two other modules.

It would be a lot of work to figure out what to drop and what to keep, and I'm pretty sure those changes would be extremely unpopular. But I think it would be the only way forward.

So what about keeping the vmware_guest in a c.vmware and creating better modules in vmware.vmware?
We also have created follwoing collection:
https://github.com/redhat-cop/cloud.vmware_ops

There is a role called provison_vm, which basically just calls the c.vmware_guest module. So the interface of the role is very similar to interface of c.vmware_guest. With that we can in future change the implementation of the role to use new modules in vmware.vmware without changing the interface and keep the similar functionality as c.vmware_guest. Maybe with such an approach we won't upset too many users. I mean they can switch from c.vmware_guest to vmware_ops.provision_vm and use similar variables, and don't need to split their playbook to multiple tasks.

@mariolenz
Copy link
Collaborator

So what about keeping the vmware_guest in a c.vmware and creating better modules in vmware.vmware?

At the moment, I guess this would be the best idea. Although I'm not sure if you really have to create (all) those modules. I guess there are already some in c.vmware that we could move. I'll try to find the time to have a closer look at this, but probably not before the weekend.

Of course, this would break the

goal to keep the interface of the modules, so users just rename the namespace and keep the rest of parameters as is, so we keep the migration path for them as simple as possible

Maybe we should discuss this further when I've found the time to make some proposals. But, as I've said, I don't think I'll find the time to do this before the weekend.

@mariolenz
Copy link
Collaborator

This module is so complex that even thinking about how to improve it hurts...

OK, I should say that the main problem is that c.vmware_guest tries to do too many things at once. It not only makes sure a VM exists / doesn't exist, it also manages some basic configuration like CPUs and memory (which should be OK), can mark a VM as a template and vice versa, manages CDROMs / disks / NICs along with advanced settings / annotations / custom attributes and some other stuff.

Maybe we should strip this down considerably to creating / deleting VMs and managing some very basic configuration. I've started to play around with this idea, but it's still early days: ansible-collections/community.vmware#2052

@mariolenz
Copy link
Collaborator

There's both select_datastore and autoselect_datastore. I don't really understand this ATM.

As I've said, this module is a mess 🤷

@mariolenz
Copy link
Collaborator

Argh! The documentation says that you need to define either esxi_hostname or cluster, but there are even CI tests that don't and still work. I'm afraid changing this would be a breaking change 😢

Burn it!

@machacekondra
Copy link
Collaborator Author

Argh! The documentation says that you need to define either esxi_hostname or cluster, but there are even CI tests that don't and still work. I'm afraid changing this would be a breaking change 😢

Hmm, I see documentation says it's required just in description not in documentation metadata, also it's not in AnsibleModule metadata. So I wonder how/when the module crashes if it really is required. If we put required field, and it crashed before, so now it will fail earlier, so we don't break anything, just fix proper error messages actually.

There's both select_datastore and autoselect_datastore. I don't really understand this ATM.

Hmm, I can see why users may like this feature, but from maintainers perspective it's really terrible thing.

What are your feelings so far, do you still think we should split the vmware_guest module, or should we start with something more simple, when we have the vmware.vmware collection ready now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.