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.

Sign up for GitHub

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

Jump to bottom

Add possiblity to tell vsphere_copy which diskformat is being uploaded #1995

Merged
merged 5 commits into from
Feb 15, 2024
Merged

Add possiblity to tell vsphere_copy which diskformat is being uploaded #1995

merged 5 commits into from
Feb 15, 2024

Conversation

Klaas-
Copy link
Contributor

@Klaas- Klaas- commented Feb 8, 2024

SUMMARY

Add possiblity to tell vsphere_copy which diskformat is being uploaded

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

vsphere_copy

ADDITIONAL INFORMATION

Related issue: vmware/pyvmomi#1064

- name: Copy file to datastore using other_system
  community.vmware.vsphere_copy:
    hostname: '{{ vcenter_hostname }}'
    username: '{{ vcenter_username }}'
    password: '{{ vcenter_password }}'
    src: /other/local/streamOptimized.vmdk
    datacenter: DC2 Someplace
    datastore: datastore2
    path: disk_imports/streamOptimized.vmdk
    timeout: 360
    diskformat: StreamVmdk

@ihumster
Copy link
Collaborator

ihumster commented Feb 8, 2024

diskformat parameter cannot be just str, it must be of type an enumeration with a list of available formats (choises parameter in argument_spec) https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec

And I don't view in your code argument_spec changes.

@Klaas-
Copy link
Contributor Author

Klaas- commented Feb 8, 2024

diskformat parameter cannot be just str, it must be of type an enumeration with a list of available formats (choises parameter in argument_spec) https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec

And I don't view in your code argument_spec changes.

I actually do not know what options it has besides StreamVmdk because it's undocumented :) But I'll gladly change it to support only StreamVmdk :)

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/9d05b7206ede44598f38879fef283135

✔️ ansible-tox-linters SUCCESS in 9m 38s
✔️ build-ansible-collection SUCCESS in 10m 18s
✔️ ansible-test-cloud-integration-vcenter7_only-stable216 SUCCESS in 19m 27s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable216 SUCCESS in 18m 32s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_1_of_2 SUCCESS in 1h 01m 27s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_2_of_2 SUCCESS in 48m 50s
✔️ ansible-galaxy-importer SUCCESS in 5m 27s

@mariolenz mariolenz added needs_info This issue requires further information. Please answer any outstanding questions feature This issue/PR relates to a feature request labels Feb 11, 2024
@mariolenz mariolenz removed the needs_info This issue requires further information. Please answer any outstanding questions label Feb 12, 2024
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/15c93290638c4beabf5dbcadd5ffcb01

✔️ ansible-tox-linters SUCCESS in 10m 13s
✔️ build-ansible-collection SUCCESS in 11m 09s
✔️ ansible-test-cloud-integration-vcenter7_only-stable216 SUCCESS in 20m 31s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable216 SUCCESS in 20m 15s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_1_of_2 SUCCESS in 58m 33s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_2_of_2 SUCCESS in 51m 11s
✔️ ansible-galaxy-importer SUCCESS in 5m 01s

mariolenz
mariolenz requested changes Feb 12, 2024
View reviewed changes
Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a suggestion to improve this PR.

What do you think about this @ihumster?

Also @Klaas- Please remember to add a changelog fragment. Should be minor_changes, I think.

plugins/modules/vsphere_copy.py Outdated Show resolved Hide resolved
plugins/modules/vsphere_copy.py Outdated Show resolved Hide resolved
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/876b710a8fb44b1e88b7e1abf46acaca

✔️ ansible-tox-linters SUCCESS in 9m 56s
✔️ build-ansible-collection SUCCESS in 10m 42s
✔️ ansible-test-cloud-integration-vcenter7_only-stable216 SUCCESS in 24m 44s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable216 SUCCESS in 22m 36s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_1_of_2 SUCCESS in 1h 02m 08s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_2_of_2 SUCCESS in 54m 24s
✔️ ansible-galaxy-importer SUCCESS in 4m 24s

@mariolenz
Copy link
Collaborator

@Klaas- Please remember to add a changelog fragment. Should be minor_changes, I think.

How about changelogs/fragments/1995-vsphere_copy.yml:

minor_changes:
  - vsphere_copy - Add parameter to tell vsphere_copy which diskformat is being uploaded (https://github.com/ansible-collections/community.vmware/pull/1995).

?

@Klaas-
Copy link
Contributor Author

Klaas- commented Feb 15, 2024

@Klaas- Please remember to add a changelog fragment. Should be minor_changes, I think.

How about changelogs/fragments/1995-vsphere_copy.yml:

minor_changes:
  - vsphere_copy - Add parameter to tell vsphere_copy which diskformat is being uploaded (https://github.com/ansible-collections/community.vmware/pull/1995).

?

that looks good to me, I've added the changelog fragment

@Klaas- Klaas- requested a review from mariolenz February 15, 2024 02:30
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/3c54865bbadd4d43b330e27ff32ed68d

✔️ ansible-tox-linters SUCCESS in 11m 45s
✔️ build-ansible-collection SUCCESS in 12m 16s
✔️ ansible-test-cloud-integration-vcenter7_only-stable216 SUCCESS in 23m 37s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable216 SUCCESS in 22m 36s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_1_of_2 SUCCESS in 1h 03m 45s
❌ ansible-test-cloud-integration-vcenter7_1esxi-stable216_2_of_2 NODE_FAILURE Node request 200-0006918121 failed in 0s
✔️ ansible-galaxy-importer SUCCESS in 5m 26s

@Klaas-
Copy link
Contributor Author

Klaas- commented Feb 15, 2024

recheck

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/ad5d190070244803b0541eb8e11a8e8d

✔️ ansible-tox-linters SUCCESS in 9m 34s
✔️ build-ansible-collection SUCCESS in 10m 21s
✔️ ansible-test-cloud-integration-vcenter7_only-stable216 SUCCESS in 21m 51s
✔️ ansible-test-cloud-integration-vcenter7_2esxi-stable216 SUCCESS in 20m 13s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_1_of_2 SUCCESS in 53m 44s
✔️ ansible-test-cloud-integration-vcenter7_1esxi-stable216_2_of_2 SUCCESS in 1h 06m 50s
✔️ ansible-galaxy-importer SUCCESS in 5m 03s

mariolenz
mariolenz approved these changes Feb 15, 2024
View reviewed changes
Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @Klaas-!

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build succeeded (gate pipeline).
https://ansible.softwarefactory-project.io/zuul/buildset/5f5587775a184ae1b19acc1dbb8363e7

✔️ ansible-tox-linters SUCCESS in 9m 49s
✔️ build-ansible-collection SUCCESS in 7m 48s
✔️ ansible-galaxy-importer SUCCESS in 3m 32s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 322f0c2 into ansible-collections:main Feb 15, 2024
13 checks passed
@Klaas-
Copy link
Contributor Author

Klaas- commented Feb 21, 2024

Just as a follow up: I am having inconsistent success with uploading large image files, but this seems to be a python 3.10+ problem: python/cpython#110467 , with python3.9 I am seeing similar issues, but it happens less frequently. Also I saw I left some inconsistencies in spelling :) I will fix all occurrences to say "StreamVmdk" in a 2nd PR

Klaas- added a commit to Klaas-/community.vmware that referenced this pull request Feb 21, 2024
@Klaas-
Refs: ansible-collections#1995 make spelling of StreamVmdk consistent
832f183
@Klaas- Klaas- mentioned this pull request Feb 21, 2024
Merged
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Feb 21, 2024
@Klaas-
Refs: #1995 make spelling of StreamVmdk consistent (#2005)
d145030 
vsphere_copy: make spelling of StreamVmdk consistent

SUMMARY

This is a follow up to #1995
I overlooked the capitalization in some of the changes suggested by maintainers, this makes it consistent across all occurrences.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
vsphere_copy
ADDITIONAL INFORMATION

Reviewed-by: Mario Lenz <[email protected]>
@Klaas-
Copy link
Contributor Author

Klaas- commented Feb 26, 2024 •
edited
Loading

and to publicly document my workaround for the problem in python 3.10/11 (python/cpython#110467)

I use curl on cli, that seems to have a 100% successrate :)

- ansible.builtin.command:
    argv:
      - /usr/bin/curl
      - -u
      - "{{ vsphere_automation_account_username }}:{{ vsphere_automation_account_password }}"
      - "https://{{ vsphere_hostname }}/folder/{{ vmname_fqdn }}_import/{{ vmname_fqdn }}.vmdk?dsName={{ vsphere_datastore }}&dcPath={{ vsphere_datacenter }}&diskFormat=StreamVmdk"
      - -H
      - "Content-type: application/octet-stream"
      - -T
      - "/path/to/local/{{ vmname_fqdn }}.vmdk"

Will switch back to vsphere_copy once that issue is solved :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihumster ihumster ihumster left review comments

@mariolenz mariolenz mariolenz approved these changes

Assignees
No one assigned
Labels
feature This issue/PR relates to a feature request mergeit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Klaas- @ihumster @mariolenz