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

Stable vmWare VM disk creation (vsphere_guest only) #28706

Closed
wants to merge 1 commit into from

Conversation

noroutine
Copy link

@noroutine noroutine commented Aug 27, 2017

Stable vmWare VM disk creation (vsphere_guest only)

    * Create more than 15 disks during reconfigure and creation
    * Allow to specify SCSI controller and unit
    * Create needed SCSI controllers automatically
    * Fix disk ordering

04 Jan 2018:
    * rebased against latest 2.5
    * cleaned up code to have less duplicates
    * relaxed restriction to specify esxi hostname for vm creation
      VM can now be created in resource pool from scratch
    * removed buggy restriction to require force together with state
SUMMARY

Comprehensive fix for creating large arrays on vsphere. Unfortunately only for old module for now.

Background:
For large disk configurations in vm_disk field, the module failed to create disk arrays when vm was created from scratch or reconfigured, when more than 15 disks where requested. Additionally the disks where created in wrong order. Wrong here means, module uses dictionary sort and disk 10 is created as as 'Hard disk 2' for example, while natural sort should be used in the process.

While I saw several fixes merged already which try to fix the issue, none of them addressed two other, namely having predictable SCSI controller and lun setting and creating disks in predictable for human order

This pull request is supposed to fix all three issues, by introducing extra parameters: controller and unit in disk dict. This fix works for both creating from scratch or reconfiguring later

The fix falls back gracefully for simple setups with < 16 disks, and allows to omit these parameters for first 15 disks, however user still can specify them and they will be honored. For disk1 till disk15, the controller is assumed to be 0 if missing and unit number is assumed to be disk id - 1 for disk1 to disk7 and disk id for disk8 to disk15

Limitations:
Unfortunately, as it appears vsphere doesn't allow to predictably name hard disks via API and enforces it's own numbering. This means the disk ids should appear in the dict without gaps. Means you can't create disk 15 without creating first 14 disks, otherwise numbers will not match anymore and subsequent task runs may fail or behave unpredictably.

Disk numbering in vsphere begins with 1, so there should be no disk0 entry in the vm_disk. This might break for users who have it, since before vsphere created it as 'Hard disk 1', enforcing it. With this fix, disk id in vm_disk dict is used to actually match the disk in vm, so requirement for the number is much stricter.

Fixes #19326 I suppose, however there were already several patches for that version merged anyway

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

vsphere_guest

ANSIBLE VERSION
ansible 2.5.0 (vsphere_guest_disk_fix 107bd8b233) last updated 2018/01/05 01:04:01 (GMT +200)
  config file = None
  configured module search path = [u'/home/tomcat/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /opt/data/external/ansible/ansible/lib/ansible
  executable location = /opt/data/external/ansible/ansible/bin/ansible
  python version = 2.7.13 (default, Nov 24 2017, 17:33:09) [GCC 6.3.0 20170516]

ADDITIONAL INFORMATION

Way to reproduce the problem would involve playing with quite some playbooks and some screenshots, so instead of pasting tons of yaml, here is general guidance and important piece of boring code for copy:

  • Create any vm with disk array that has more than 15 disks via playbook
    or

  • Create small vm and add disks during reconfiguring

  • Observe the disk numbers assigned by vsphere are not matching dict specified for disks above 10

  • Observe error when reconfiguring for more than 15 disks

I'll simply paste the tedious piece for anyone who will test this

  vm_disk:
    disk1:
      size_gb: 20
      type: thin
      datastore: "{{ vsphere_datastore }}"
    disk2:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 1
      unit: 0
    disk3:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
    disk4:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
    disk5:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
    disk6:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
    disk7:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
    disk8:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 1
      unit: 1
    disk9:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 1
      unit: 2
    disk10:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 1
      unit: 3
    disk11:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 1
      unit: 4
    disk12:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 2
      unit: 1
    disk13:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 3
      unit: 1
    disk14:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 3
      unit: 2
    disk15:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 3
      unit: 3
    disk16:
      size_gb: 10
      type: thin
      datastore: "{{ vsphere_datastore }}"
      controller: 3
      unit: 4

@ansibot
Copy link
Contributor

ansibot commented Aug 27, 2017

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bugfix_pull_request cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. vmware VMware community labels Aug 27, 2017
@ansibot
Copy link
Contributor

ansibot commented Aug 27, 2017

The test ansible-test sanity --test import --python 2.6 failed with the following error:

lib/ansible/modules/cloud/vmware/vsphere_guest.py:323:0: ImportError: No module named natsort

The test ansible-test sanity --test import --python 2.7 failed with the following error:

lib/ansible/modules/cloud/vmware/vsphere_guest.py:323:0: ImportError: No module named natsort

The test ansible-test sanity --test import --python 3.5 failed with the following error:

lib/ansible/modules/cloud/vmware/vsphere_guest.py:323:0: ImportError: No module named 'natsort'

The test ansible-test sanity --test import --python 3.6 failed with the following error:

lib/ansible/modules/cloud/vmware/vsphere_guest.py:323:0: ModuleNotFoundError: No module named 'natsort'

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/cloud/vmware/vsphere_guest.py:840:22: W291 trailing whitespace
lib/ansible/modules/cloud/vmware/vsphere_guest.py:1491:41: W291 trailing whitespace

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 27, 2017
@noroutine noroutine force-pushed the vsphere_guest_disk_fix branch 2 times, most recently from ec8bac6 to 011b854 Compare August 27, 2017 20:57
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Aug 27, 2017
@noroutine
Copy link
Author

Removed natsort dependency and fixed trailing spaces

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 27, 2017
@noroutine noroutine force-pushed the vsphere_guest_disk_fix branch 2 times, most recently from 876f215 to ef5cace Compare August 27, 2017 22:10
@noroutine
Copy link
Author

Added some comment to vm_disk docs about numbering and warning about number gaps

@ansibot
Copy link
Contributor

ansibot commented Aug 27, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/cloud/vmware/vsphere_guest.py:102:161: E501 line too long (164 > 160 characters)
lib/ansible/modules/cloud/vmware/vsphere_guest.py:102:165: W291 trailing whitespace

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 27, 2017
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 27, 2017
@ansibot ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Jan 5, 2018
@noroutine
Copy link
Author

@digennarot can you review one more time? :)

    * Create more than 15 disks during reconfigure and creation
    * Allow to specify SCSI controller and unit
    * Create needed SCSI controllers automatically
    * Fix disk ordering

04 Jan 2018:
    * rebased against latest 2.5
    * cleaned up code to have less duplicates
    * relaxed restriction to specify esxi hostname for vm creation
      VM can now be created in resource pool from scratch
    * removed buggy restriction to require force together with state
@ansibot ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Jan 5, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 13, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. new_module This PR includes a new module. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Feb 6, 2018
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018
@Akasurde Akasurde removed the new_module This PR includes a new module. label Apr 2, 2018
@ansibot ansibot added the new_module This PR includes a new module. label Apr 2, 2018
@ansibot
Copy link
Contributor

ansibot commented May 4, 2018

@ansibot ansibot added the new_plugin This PR includes a new plugin. label May 20, 2018
@Akasurde Akasurde added the deprecated This issue/PR relates to a deprecated module. label Jun 25, 2018
@Akasurde Akasurde removed new_module This PR includes a new module. new_plugin This PR includes a new plugin. labels Oct 23, 2018
@ansibot ansibot added the new_module This PR includes a new module. label Oct 23, 2018
@ansibot
Copy link
Contributor

ansibot commented Nov 24, 2018

cc @ckotte
click here for bot help

@ansibot ansibot removed the deprecated This issue/PR relates to a deprecated module. label Feb 9, 2019
@ansibot
Copy link
Contributor

ansibot commented Feb 17, 2019

@ansibot
Copy link
Contributor

ansibot commented Feb 25, 2019

@ansibot
Copy link
Contributor

ansibot commented May 9, 2019

cc @goneri
click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Jun 2, 2019

@Akasurde
Copy link
Member

Thanks for the PR. Closing this PR since vsphere_guest is removed in 2.9 version.

@Akasurde Akasurde closed this Jun 20, 2019
@ansible ansible locked and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. cloud module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. new_module This PR includes a new module. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community. vmware VMware community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vsphere_guest unable to add additional disks
5 participants