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

VMware: return facts depending upon backing type #52638

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@bushvin
Copy link
Contributor

bushvin commented Feb 20, 2019

The class used for the backing is not identical in all cases, and do not have the same properties/methods.
Following backings exists at the time of this writing:
vim.vm.device.VirtualDisk.FlatVer1BackingInfo
vim.vm.device.VirtualDisk.FlatVer2BackingInfo
vim.vm.device.VirtualDisk.LocalPMemBackingInfo
vim.vm.device.VirtualDisk.PartitionedRawDiskVer2BackingInfo
vim.vm.device.VirtualDisk.RawDiskMappingVer1BackingInfo
vim.vm.device.VirtualDisk.RawDiskVer2BackingInfo
vim.vm.device.VirtualDisk.SeSparseBackingInfo
vim.vm.device.VirtualDisk.SparseVer1BackingInfo
vim.vm.device.VirtualDisk.SparseVer2BackingInfo

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

Not all Backings are equal
The class used for the backing is not identical in all cases, and do not have the same properties/methods.
Following backings exists at the time of this writing:
vim.vm.device.VirtualDisk.FlatVer1BackingInfo
vim.vm.device.VirtualDisk.FlatVer2BackingInfo
vim.vm.device.VirtualDisk.LocalPMemBackingInfo
vim.vm.device.VirtualDisk.PartitionedRawDiskVer2BackingInfo
vim.vm.device.VirtualDisk.RawDiskMappingVer1BackingInfo
vim.vm.device.VirtualDisk.RawDiskVer2BackingInfo
vim.vm.device.VirtualDisk.SeSparseBackingInfo
vim.vm.device.VirtualDisk.SparseVer1BackingInfo
vim.vm.device.VirtualDisk.SparseVer2BackingInfo
@bushvin

This comment has been minimized.

Copy link
Contributor Author

bushvin commented Feb 20, 2019

No idea whether it's either a Bugfix or Feature PR...

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 20, 2019

@Akasurde Akasurde changed the title Not all Backings are equal VMware: return facts depending upon backing type Feb 20, 2019

@Akasurde Akasurde self-requested a review Feb 20, 2019

@Akasurde Akasurde removed the needs_triage label Feb 20, 2019

Per the request of my employer
Per the request of my employer. No other changes required.
@bushvin

This comment has been minimized.

Copy link
Contributor Author

bushvin commented Feb 20, 2019

Sorry about that last commit. They initially wanted nothing, but they changed thzir mind inbetween

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Feb 25, 2019

@bushvin Thanks for the PR. Let me know once you are done with the requested changes. Thanks.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 25, 2019

update
- Added update notification to notes
- Extended examples to match new return information (FlatVer2, RawDiskMappingVer1)
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 25, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest_disk_facts.py:113:13: E313 RETURN is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest_disk_facts.py:113:13: error RETURN: syntax error: expected ',' or '}', but got '<scalar>'

click here for bot help

@ansibot ansibot added the ci_verified label Feb 25, 2019

@ansibot ansibot removed the ci_verified label Feb 25, 2019

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Mar 4, 2019

controller_key=disk.controllerKey,
unit_number=disk.unitNumber,
capacity_in_kb=disk.capacityInKB,
capacity_in_bytes=disk.capacityInBytes,
)
if isinstance(disk.backing, vim.vm.device.VirtualDisk.FlatVer1BackingInfo):

This comment has been minimized.

@jeking3

jeking3 Mar 4, 2019

Contributor

Just a commentary on the project's direction in general (not the PR author's) - having to transform results from the well-documented vSphere API to an arbitrary format per ansible module leads to chaos.

This style of rewriting results is unnecessary if you return the vSphere API object in json form... the vmware_guest_facts module in devel (and 2.8) can do this already, including returning complete details about the guest, including the disk backings, with all their details and without rewriting them.

This comment has been minimized.

@Akasurde

Akasurde Mar 4, 2019

Member

@jeking3 I totally agree with you. But before the change we didn't had an option to get facts about any VMware object from vCenter API. We can come up with strong proposal (not here of-course) and discuss in meeting so that we can arrive at roadmap kind of decision. Thanks for pointing this out.

This comment has been minimized.

@alongchamps

alongchamps Mar 5, 2019

I'm in agreement that we should have a discussion on this. The more we keep in line with the VMware API, the easier it will be to traverse objects.

@jeking3

jeking3 approved these changes Mar 4, 2019

Copy link
Contributor

jeking3 left a comment

It would be nice to see the integration test (if there is one) updated to check the new field value for the backing type.

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Mar 4, 2019

It would be nice to see the integration test (if there is one) updated to check the new field value for the backing type.

I tried govcsim with this module but it does not emulate different types of disk types.

@ansibot ansibot added the stale_ci label Mar 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.