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: vmware_tools connection plugin #55059

Merged
merged 5 commits into from
Apr 11, 2019

Conversation

jamescassell
Copy link
Contributor

SUMMARY

I've updated the referenced PR

Fixes: #47072

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

vmware_tools

ADDITIONAL INFORMATION

At one point there was a wiki page attempting to document and standardize these going forward, but I'm not sure where that is anymore. I think the only things we all agree on are ansible_user and ansible_password defaulting for the vm username and password. smile

Do you mean this - https://github.com/ansible/community/wiki/VMware:-standardization

I've integrated the changes from TLS standardization and connection_var standardization. I also added support for the VI_* env vars standardized by vmware sdk.

I could use some help getting started or some examples of what this needs to look like. Is there a good reference for this in the existing ansible codebase?

I am sure that we can do integration test as govcsim does not emulate actual OS. So unit test will be more than sufficient for this change. You may want to follow existing connection plugin testcases - https://github.com/ansible/ansible/tree/devel/test/units/plugins/connection

I'm punting this for now. I'd want to do an integration test, but that requires a full VMware setup.

  1. End-user documentation

I assumed that meant something like the winrm connection page, but I thought that would be autogenerated from the DOCUMENTATION block. Either way, do we need more?

Yes, we can add documentation here = https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/vmware/ as how-to-use connection plugin.

I added an EXAMPLES block.

I'm not sure how to go about doing that here, it's potentially something different altogether, see the discussion in #43428.
I believe the plugin works, as (manually) tested by myself and at least a couple of others in this thread. I think we just need to decide what it needs for the 2.8 release. I'm willing to finish it up, but I need use some clearer definitions of what finished looks like.

Not sure about this. But I think if we can get initial version in tree, we can always improvise things. Its better to have something than nothing. :)

This PR intends to show what 'finished' looks like.


@jamescassell
Copy link
Contributor Author

@Akasurde @dericcrago @pdellaert @Im0 @tchernomax @dagwieers @jborean93 @nitzmahone @verdel PTAL. This is a cleanup of #47072 addressing most of the feedback.

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. new_plugin This PR includes a new plugin. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 9, 2019
@Akasurde Akasurde requested review from Akasurde and jillr April 10, 2019 03:32
@Akasurde Akasurde self-assigned this Apr 10, 2019
@Akasurde Akasurde removed the needs_triage Needs a first human triage before being processed. label Apr 10, 2019
@Akasurde Akasurde changed the title vmware_tools connection plugin VMware: vmware_tools connection plugin Apr 10, 2019
@Akasurde
Copy link
Member

Akasurde commented Apr 10, 2019

@ckotte @jeking3 @Tomorrow9 @alongchamps @adarobin @goneri Could you please review this ?

lib/ansible/plugins/connection/vmware_tools.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/vmware_tools.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/vmware_tools.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/vmware_tools.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/vmware_tools.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/vmware_tools.py Outdated Show resolved Hide resolved
@Akasurde
Copy link
Member

@jamescassell Thanks for working on this. Let me know if you need any help with this.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Apr 10, 2019
@Akasurde
Copy link
Member

@jamescassell I added changes here - jamescassell#1 feel free to modify.

@jamescassell
Copy link
Contributor Author

@Akasurde I added your changes, re-added the VI_* env vars, and then squashed to one commit per contibutor so ansibot will be happy to merge it once approved.

@jamescassell
Copy link
Contributor Author

@Akasurde I've integrated the last two of your suggestions. (Shippable seems broken, however... which seems unrelated)

@jamescassell
Copy link
Contributor Author

@jamescassell Would you be able to address review comments ? Thursday is feature freeze for 2.8. Let me know. Thanks.

Looks like it boils down to:

  • adding self.get_option('executable') preferably with a ansible_vmware_tools_executable var for configuration
  • dropping silence_tls_warnings
  • AnsibleError -> AnsibleConnection error
  • swap python requests for Request from module_utils/urls.py

I can likely get the first 3, but I need to know what the default value should be for executable

I'm not sure if I can just drop/replace requests.get with Requests.get ?

I won't be able to test any changes I make until 7 hours from now. (I have, of course, tested the PR as-is.)

@jamescassell jamescassell force-pushed the vmware_tools_conn branch 2 times, most recently from 92c1841 to feda014 Compare April 10, 2019 22:46
jamescassell and others added 2 commits April 10, 2019 18:49
- connection_address -> vmware_host

- connection_username -> vmware_user

- connection_password -> vmware_password

- connection_verify_ssl -> validate_certs

- connection_ignore_ssl_warnings -> silence_tls_warnings

- ansible_vmware_tools_vm_path -> ansible_vmware_guest_path

- standardize user/pass vars

- fallback to default ansible conneciton vars

- accept VMware standard env vars:
https://code.vmware.com/docs/6536/vsphere-sdk-for-perl-programming-guide/doc/GUID-66555F76-570A-4C76-BEA7-6C371BF685D6.html

- note lack of "become" support

- add example usage

- more reasonable default sleep interval

- auto-silence tls warnings if validate_certs=false

- get_option for executable

- remove unsafe 'makedirs_safe'
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@jamescassell
Copy link
Contributor Author

jamescassell commented Apr 10, 2019

@jamescassell Would you be able to address review comments ? Thursday is feature freeze for 2.8. Let me know. Thanks.

Looks like it boils down to:

  • adding self.get_option('executable') preferably with a ansible_vmware_tools_executable var for configuration

Done.

  • dropping silence_tls_warnings

Changed default to true as some folks think it's useful.

  • AnsibleError -> AnsibleConnection error

Done.

  • swap python requests for Request from module_utils/urls.py

Will not do, as urls.py doesn't support chunked downloads and the rest of vmware in ansible also requires requests.

Otherwise, I think I've addressed all feedback. I've also tested these changes in my environment, and everything seems to be functional.

ready_for_review

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 10, 2019
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
DOCUMENTATION = """
author: Deric Crago <deric.crago@gmail.com>
connection: vmware_tools
short_description: Execute tasks inside a VM via VMware Tools
Copy link
Member

Choose a reason for hiding this comment

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

i would mention this plugin is Windows specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plugin works for both linux and windows.

super(Connection, self).__init__(*args, **kwargs)
if hasattr(self, "_shell") and self._shell.SHELL_FAMILY == "powershell":
self.module_implementation_preferences = (".ps1", ".exe", "")
self.become_methods = ["runas"]
Copy link
Member

Choose a reason for hiding this comment

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

these settings make this a 'windows only' connection , but examples above suggest otherwise, which is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It supports both. If SHELL_FAMILY=powershell, it does this special magic. If not, it "just works" for Linux otherwise.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Apr 11, 2019
@jamescassell
Copy link
Contributor Author

Thanks @Akasurde for the fixes! I've also addressed the fix suggested by @bcoca of also enabling ini and env for the executable setting.

Let me know if I should re-squash to one commit per author.

ready_for_review

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 11, 2019
@Akasurde Akasurde merged commit 2b8cef3 into ansible:devel Apr 11, 2019
@jamescassell
Copy link
Contributor Author

@Akasurde thanks for all your help on this and for merging! @dericcrago thanks for writing it to begin with!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. new_plugin This PR includes a new plugin. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants