ansible-collections / community.vmware

Ansible Collection for VMware
GNU General Public License v3.0
348 stars 341 forks source link

VMware: vmware_guest multiple CDROM implementation breaks backwards compatibility and other issues #334

Open Akasurde opened 4 years ago

Akasurde commented 4 years ago

From @oatakan on Nov 01, 2019 21:25

SUMMARY

There are a couple of issues with the new multiple CD-ROM implementation:

  1. breaks compatibility, controller_number was assumed to be 0 (default) if not specified in previous releases, now it's mandatory attribute so existing playbooks with vmware_guest and cdrom attribute specified will fail unless updated with controller_number and unit_number attributes.
  2. user is asked to manage cdrom states individually which does not follow the implementation for disks. this is unnecessarily complicated, you should just specify what list of cdroms should exist, vmware_guest module would implement the actual vs desired state difference. shouldn't need to individually specify state: present/absent for each cdrom in the list. This forces users to know the original state of the system (as in which cdrom already attached) to be able to remove any of them which goes against idempotency.
  3. user is forced to know the controller_number and unit_number which shouldn't be the case

Please see this PR that does not have these limitations: https://github.com/ansible/ansible/pull/58951

ISSUE TYPE
COMPONENT NAME

vmware_guest

ANSIBLE VERSION
ansible 2.9.0
  config file = /Users/oatakan/.ansible.cfg
  configured module search path = [u'/Users/oatakan/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/ansible
  executable location = /Library/Frameworks/Python.framework/Versions/2.7/bin/ansible
  python version = 2.7.15 (v2.7.15:ca079a3ea3, Apr 29 2018, 20:59:26) [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)]
CONFIGURATION
OS / ENVIRONMENT
STEPS TO REPRODUCE

Here is the recommended multiple cdrom approach as it is implemented by https://github.com/ansible/ansible/pull/58951:


- name: attach multiple cdroms
  vmware_guest:
    hostname: "{{ vcenter_hostname }}"
    username: "{{ vcenter_username }}"
    password: "{{ vcenter_password }}"
    validate_certs: no
    name: testvm_2
    cdrom:
      - type: iso
        iso_path: "[datastore1] file1.iso"
      - type: iso
        iso_path: "[datastore1] file2.iso"

- name: detach one of the cdroms
  vmware_guest:
    hostname: "{{ vcenter_hostname }}"
    username: "{{ vcenter_username }}"
    password: "{{ vcenter_password }}"
    validate_certs: no
    name: testvm_2
    cdrom:
      - type: iso
        iso_path: "[datastore1] file2.iso"

instead of (the current new implementation in 2.9):

- name: attach multiple cdroms
  vmware_guest:
    hostname: "{{ vcenter_hostname }}"
    username: "{{ vcenter_username }}"
    password: "{{ vcenter_password }}"
    validate_certs: no
    name: testvm_2
    cdrom:
      - type: iso
        iso_path: "[datastore1] file1.iso"
        controller_number: 0
        unit_number: 1
      - type: iso
        iso_path: "[datastore1] file2.iso"
        controller_number: 0
        unit_number: 2

- name: detach one of the cdroms
  vmware_guest:
    hostname: "{{ vcenter_hostname }}"
    username: "{{ vcenter_username }}"
    password: "{{ vcenter_password }}"
    validate_certs: no
    name: testvm_2
    cdrom:
      - type: iso
        iso_path: "[datastore1] file1.iso"
        controller_number: 0
        unit_number: 1
        state: absent
      - type: iso
        iso_path: "[datastore1] file2.iso"
        controller_number: 0
        unit_number: 2
EXPECTED RESULTS
ACTUAL RESULTS

Copied from original issue: ansible/ansible#64303

Tomorrow9 commented 4 years ago

Hi @Akasurde, for issue 3: "3. user is forced to know the controller_number and unit_number which shouldn't be the case"

is introduced with adding CDROM attaching to SATA controller support, the same as disks without these new parameters then don't know attach to which IDE/SATA controller.

ansibullbot commented 4 years ago

cc @goneri @lparkes @nerzhul @pdellaert @pgbidkar @warthog9 click here for bot help