vmware_guest_disk: Add iolimit modifications on an existing disk with… #1466

yo000 · 2022-09-21T13:27:45Z

SUMMARY

This commit add iolimit modifications on an existing disk without changing size.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

plugin community.vmware.vmware_guest_disk

ADDITIONAL INFORMATION

As of today, one can't change an existing disk iops limit and/or shares level without also changing the disk size.
Idempotency is only done on disk size, so task will "ok" whatever iolimit is set to.
This commit fix that

- name: Gather disk info from virtual machine using name
  community.vmware.vmware_guest_disk_info:
    datacenter: "{{ vmware_datacenter }}"
    hostname: "{{ vmware_vcenter }}"
    username: "{{ vmware_username }}"
    password: "{{ vmware_password }}"
    name: "{{ inventory_hostname }}"
  delegate_to: localhost
  register: disk_info

- name: Apply iops
  community.vmware.vmware_guest_disk:
    datacenter: "{{ vmware_datacenter }}"
    hostname: "{{ vmware_vcenter }}"
    username: "{{ vmware_username }}"
    password: "{{ vmware_password }}"
    name: "{{ inventory_hostname }}"
    disk:
      - filename: '{{ disk_info.guest_disk_info["0"].backing_filename }}'
        size_gb: "{{ vm_disk_size }}"
        type: "{{ vm_disk_type }}"
        datastore: "{{ vm_datastore }}"
        disk_mode: '{{ disk_info.guest_disk_info["0"].backing_diskmode }}'
        scsi_controller: '{{ disk_info.guest_disk_info["0"].controller_bus_number }}'
        unit_number: '{{ disk_info.guest_disk_info["0"].unit_number }}'
        iolimit:
          limit: "{{ vm_disk_iops }}"
          shares:
            level: normal

…out changing size

softwarefactory-project-zuul · 2022-09-21T19:01:51Z

Build failed.

❌ ansible-tox-linters FAILURE in 4m 20s
✔️ build-ansible-collection SUCCESS in 4m 21s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 28s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 09s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 8m 12s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 7m 34s
✔️ ansible-test-units-community-vmware-python38 SUCCESS in 5m 03s
✔️ ansible-test-cloud-integration-vcenter7_only-stable212 SUCCESS in 21m 56s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable212 SUCCESS in 19m 08s
❌ ansible-test-cloud-integration-vcenter7_1esxi-stable212_1_of_2 RETRY_LIMIT in 1h 01m 28s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_2_of_2 SUCCESS in 45m 17s
✔️ ansible-galaxy-importer SUCCESS in 4m 21s

plugins/modules/vmware_guest_disk.py

softwarefactory-project-zuul · 2022-09-22T08:30:31Z

Build failed.

❌ ansible-tox-linters FAILURE in 4m 26s
❌ build-ansible-collection FAILURE in 3m 09s
⚠️ ansible-test-sanity-docker-devel SKIPPED (non-voting)
⚠️ ansible-test-sanity-docker-milestone SKIPPED
⚠️ ansible-test-sanity-docker-stable-2.12 SKIPPED
⚠️ ansible-test-sanity-docker-stable-2.13 SKIPPED
⚠️ ansible-test-units-community-vmware-python38 SKIPPED
⚠️ ansible-test-cloud-integration-vcenter7_only-stable212 SKIPPED
⚠️ ansible-test-cloud-integration-vcenter7_2esxi-stable212 SKIPPED
⚠️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_1_of_2 SKIPPED
⚠️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_2_of_2 SKIPPED
⚠️ ansible-galaxy-importer SKIPPED

plugins/modules/vmware_guest_disk.py

softwarefactory-project-zuul · 2022-09-22T09:49:31Z

Build failed.

✔️ ansible-tox-linters SUCCESS in 4m 20s
✔️ build-ansible-collection SUCCESS in 5m 34s
❌ ansible-test-sanity-docker-devel FAILURE in 7m 22s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 12s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 8m 42s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 8m 35s
✔️ ansible-test-units-community-vmware-python38 SUCCESS in 6m 08s
✔️ ansible-test-cloud-integration-vcenter7_only-stable212 SUCCESS in 18m 45s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable212 SUCCESS in 17m 34s
❌ ansible-test-cloud-integration-vcenter7_1esxi-stable212_1_of_2 FAILURE in 24m 02s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_2_of_2 SUCCESS in 43m 36s
✔️ ansible-galaxy-importer SUCCESS in 4m 01s

mariolenz · 2022-09-23T13:47:46Z

@yo000 Your changes are buggy. The module fails our integration tests (see the last CI run) here:

community.vmware/tests/integration/targets/vmware_guest_disk/tasks/main.yml

Line 169 in c660ecd

- name: Update disk for custom IO limits in IO Limits

It seems to work fine when changing iolimit, but it fails when changing this and the size of the disk.

Additionaly, I feel there are some more problems although I can't really put my finger on them. One thing is that there is a function to set iolimit that is used when the disk size changes:

community.vmware/plugins/modules/vmware_guest_disk.py

Lines 601 to 619 in c660ecd

    
           def get_ioandshares_diskconfig(self, disk_spec, disk): 
        
               io_disk_spec = vim.StorageResourceManager.IOAllocationInfo() 
        
               if 'iolimit' in disk: 
        
                   io_disk_spec.limit = disk['iolimit']['limit'] 
        
                   if 'shares' in disk['iolimit']: 
        
                       shares_spec = vim.SharesInfo() 
        
                       shares_spec.level = disk['iolimit']['shares']['level'] 
        
                       if shares_spec.level == 'custom': 
        
                           shares_spec.shares = disk['iolimit']['shares']['level_value'] 
        
                       io_disk_spec.shares = shares_spec 
        
                   disk_spec.device.storageIOAllocation = io_disk_spec 
        
               if 'shares' in disk: 
        
                   shares_spec = vim.SharesInfo() 
        
                   shares_spec.level = disk['shares']['level'] 
        
                   if shares_spec.level == 'custom': 
        
                       shares_spec.shares = disk['shares']['level_value'] 
        
                   io_disk_spec.shares = shares_spec 
        
                   disk_spec.device.storageIOAllocation = io_disk_spec 
        
               return disk_spec

I think it might be worth (re-) using.

Something like:

if disk['iolimit']['limit'] != disk_spec.device.storageIOAllocation.limit or disk['iolimit']['shares']['level_value'] != disk_spec.device.storageIOAllocation.shares.shares
    disk_spec.operation = vim.vm.device.VirtualDeviceSpec.Operation.edit
    disk_spec = self.get_ioandshares_diskconfig(disk_spec, disk)
    disk_change = True

And then remove self.config_spec.deviceChange.append(disk_spec), disk_change_list.append(disk_change) and results['disk_changes'][disk['disk_index']] = "Disk reconfigured." from here:

community.vmware/plugins/modules/vmware_guest_disk.py

Lines 737 to 753 in fb5fd3e

    
           # If this is an RDM ignore disk size 
        
           if disk['disk_type'] != 'rdm': 
        
               if disk['size'] < disk_spec.device.capacityInKB: 
        
                   self.module.fail_json(msg="Given disk size at disk index [%s] is smaller than found" 
        
                                             " (%d < %d). Reducing disks is not allowed." 
        
                                             % (disk['disk_index'], disk['size'], 
        
                                                 disk_spec.device.capacityInKB)) 
        
               if disk['size'] != disk_spec.device.capacityInKB: 
        
                   # set the operation to edit so that it knows to keep other settings 
        
                   disk_spec.operation = vim.vm.device.VirtualDeviceSpec.Operation.edit 
        
                   if disk['disk_type'] != 'vpmemdisk': 
        
                       disk_spec = self.get_ioandshares_diskconfig(disk_spec, disk) 
        
                   disk_spec.device.capacityInKB = disk['size'] 
        
                   self.config_spec.deviceChange.append(disk_spec) 
        
                   disk_change = True 
        
                   disk_change_list.append(disk_change) 
        
                   results['disk_changes'][disk['disk_index']] = "Disk reconfigured."

But do it later when there has been a change detected for IO limits, shares or disk size:

if disk_change:
    self.config_spec.deviceChange.append(disk_spec)
    disk_change_list.append(disk_change)
    results['disk_changes'][disk['disk_index']] = "Disk reconfigured."

This is still very crude and lacking some safety checks and similar, but what do you think?

yo000 · 2022-09-27T11:54:19Z

@mariolenz thank you for reviewing. I did not pay enough attention to get_ioandshares_diskconfig!
Changes were made as you suggest, and that fixed the "modify size and iolimit" bug.
I just had to keep the somewhat complex check on disk['iolimit']['shares']['level'] to satisfy idempotency.

softwarefactory-project-zuul · 2022-09-27T12:39:51Z

Build failed.

✔️ ansible-tox-linters SUCCESS in 4m 39s
✔️ build-ansible-collection SUCCESS in 4m 23s
❌ ansible-test-sanity-docker-devel FAILURE in 7m 42s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 12m 04s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 7m 49s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 8m 11s
✔️ ansible-test-units-community-vmware-python38 SUCCESS in 6m 21s
✔️ ansible-test-cloud-integration-vcenter7_only-stable212 SUCCESS in 17m 26s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable212 SUCCESS in 18m 21s
❌ ansible-test-cloud-integration-vcenter7_1esxi-stable212_1_of_2 FAILURE in 16m 12s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_2_of_2 SUCCESS in 44m 56s
✔️ ansible-galaxy-importer SUCCESS in 4m 03s

mariolenz · 2022-09-27T16:28:33Z

recheck

softwarefactory-project-zuul · 2022-09-27T19:00:29Z

Build succeeded.

✔️ ansible-tox-linters SUCCESS in 4m 14s
✔️ build-ansible-collection SUCCESS in 4m 09s
❌ ansible-test-sanity-docker-devel FAILURE in 7m 23s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 7m 39s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 8m 22s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 8m 11s
✔️ ansible-test-units-community-vmware-python38 SUCCESS in 6m 27s
✔️ ansible-test-cloud-integration-vcenter7_only-stable212 SUCCESS in 17m 26s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable212 SUCCESS in 18m 42s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_1_of_2 SUCCESS in 44m 36s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_2_of_2 SUCCESS in 52m 40s
✔️ ansible-galaxy-importer SUCCESS in 4m 03s

mariolenz · 2022-09-28T13:37:14Z

Could you please add a changelog fragment, like this one? Maybe something like:

minor_changes:
  - vmware_guest_disk_info - Adding `iolimit` modifications of an existing disk without changing size (https://github.com/ansible-collections/community.vmware/pull/1466).

or similar. Rest LGTM!

softwarefactory-project-zuul · 2022-09-29T09:29:30Z

Build succeeded.

✔️ ansible-tox-linters SUCCESS in 5m 07s
✔️ build-ansible-collection SUCCESS in 4m 10s
❌ ansible-test-sanity-docker-devel FAILURE in 7m 20s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 8m 38s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 8m 24s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 7m 55s
❌ ansible-test-sanity-docker-stable-2.14 FAILURE in 7m 37s (non-voting)
✔️ ansible-test-units-community-vmware-python38 SUCCESS in 6m 27s
✔️ ansible-test-cloud-integration-vcenter7_only-stable212 SUCCESS in 18m 10s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable212 SUCCESS in 20m 10s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_1_of_2 SUCCESS in 47m 51s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable212_2_of_2 SUCCESS in 53m 54s
✔️ ansible-galaxy-importer SUCCESS in 4m 14s

mariolenz · 2022-09-29T11:58:20Z

Thanks @yo000!

softwarefactory-project-zuul · 2022-09-29T12:12:42Z

Build succeeded (gate pipeline).

✔️ ansible-tox-linters SUCCESS in 4m 09s
✔️ build-ansible-collection SUCCESS in 3m 28s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 7m 39s
❌ ansible-test-sanity-docker-stable-2.14 FAILURE in 9m 08s (non-voting)
✔️ ansible-test-units-community-vmware-python38 SUCCESS in 7m 13s
✔️ ansible-galaxy-importer SUCCESS in 4m 08s

vmware_guest_disk: Backport #1466 SUMMARY This commit adds iolimit modifications on an existing disk without changing size. ISSUE TYPE Feature Pull Request COMPONENT NAME vmware_guest_disk ADDITIONAL INFORMATION Backport of #1466

yo000 added 2 commits September 21, 2022 15:21

vmware_guest_disk: Add iolimit modifications on an existing disk with…

0cd9296

…out changing size

style updates (tox compliance)

f681ea3

mariolenz reviewed Sep 22, 2022

View reviewed changes

plugins/modules/vmware_guest_disk.py Outdated Show resolved Hide resolved

Fix W504 line break after binary operator

b3c409a

mariolenz reviewed Sep 22, 2022

View reviewed changes

plugins/modules/vmware_guest_disk.py Outdated Show resolved Hide resolved

Fix E502: the backslash is redundant between brackets

4004387

mariolenz reviewed Sep 22, 2022

View reviewed changes

plugins/modules/vmware_guest_disk.py Outdated Show resolved Hide resolved

Fix E999 SyntaxError: invalid syntax

fb5fd3e

Fix bug when changing iolimit and size, use get_ioandshares_diskconfig()

2c2dce3

changelog update

31e651a

mariolenz added feature This issue/PR relates to a feature request mergeit labels Sep 29, 2022

softwarefactory-project-zuul bot merged commit 1dab1f7 into ansible-collections:main Sep 29, 2022

mariolenz mentioned this pull request Sep 29, 2022

vmware_guest_disk: Backport #1466 #1478

Merged

renovate bot mentioned this pull request Oct 10, 2022

Update dependency community.vmware to v3 - autoclosed Fredouye/argocd#11

Closed

1 task

slauger mentioned this pull request Oct 11, 2022

fix(deps): update galaxy-collection dependency community.vmware to v3 - autoclosed slauger/openshift-sdk#60

Closed

1 task

slauger mentioned this pull request Oct 24, 2022

fix(deps): update galaxy-collection dependency community.vmware to v3 - autoclosed slauger/openshift-sdk#68

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vmware_guest_disk: Add iolimit modifications on an existing disk with… #1466

vmware_guest_disk: Add iolimit modifications on an existing disk with… #1466

yo000 commented Sep 21, 2022

softwarefactory-project-zuul bot commented Sep 21, 2022

softwarefactory-project-zuul bot commented Sep 22, 2022

softwarefactory-project-zuul bot commented Sep 22, 2022

mariolenz commented Sep 23, 2022

yo000 commented Sep 27, 2022

softwarefactory-project-zuul bot commented Sep 27, 2022

mariolenz commented Sep 27, 2022

softwarefactory-project-zuul bot commented Sep 27, 2022

mariolenz commented Sep 28, 2022

softwarefactory-project-zuul bot commented Sep 29, 2022

mariolenz commented Sep 29, 2022

softwarefactory-project-zuul bot commented Sep 29, 2022

vmware_guest_disk: Add iolimit modifications on an existing disk with… #1466

vmware_guest_disk: Add iolimit modifications on an existing disk with… #1466

Conversation

yo000 commented Sep 21, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

softwarefactory-project-zuul bot commented Sep 21, 2022

softwarefactory-project-zuul bot commented Sep 22, 2022

softwarefactory-project-zuul bot commented Sep 22, 2022

mariolenz commented Sep 23, 2022

yo000 commented Sep 27, 2022

softwarefactory-project-zuul bot commented Sep 27, 2022

mariolenz commented Sep 27, 2022

softwarefactory-project-zuul bot commented Sep 27, 2022

mariolenz commented Sep 28, 2022

softwarefactory-project-zuul bot commented Sep 29, 2022

mariolenz commented Sep 29, 2022

softwarefactory-project-zuul bot commented Sep 29, 2022