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: New module vmware_host_dns #47271

Closed
wants to merge 1 commit into from

Conversation

ckotte
Copy link
Contributor

@ckotte ckotte commented Oct 18, 2018

SUMMARY

vmware_host_dns is a drop-in replacement for the old module vmware_dns_config.

Rather than removing one module, and introducing a new module, we are replacing the old module with a compatible new module, but we are deprecating some of the original parameters instead. End goal is to not break existing playbooks and get rid of some parameters.

Compared to the other module, this module..:

  • configures DNS on default TCP/IP stack in host.config.network.netStackInstance instead of deprecated host.configManager.networkSystem.dnsConfig
  • enables configuration of DNS via DHCP or static
  • enables verbose message for DNS server changes
    e.g. "msg": "DNS server '10.168.1.10' added and '10.168.1.11' removed and the server sequence changed as well"
  • enables configuration of search domains
  • supports check mode
  • supports esxi_hostname and cluster_name options
  • uses the same naming convention as the other vmware_host modules
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

vmware_host_dns

ANSIBLE VERSION
ansible 2.6.3
  config file = /root/ansible-vmware/ansible.cfg
  configured module search path = [u'/root/ansible-vmware/library']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Jul 13 2018, 13:06:57) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]
ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Oct 18, 2018

Hi @ckotte, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Oct 18, 2018

@ansibot
Copy link
Contributor

ansibot commented Oct 18, 2018

@ckotte, just so you are aware we have a dedicated Working Group for vmware.
You can find other people interested in this in #ansible-vmware on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 cloud module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. vmware VMware community labels Oct 18, 2018
@ckotte
Copy link
Contributor Author

ckotte commented Oct 18, 2018

@dagwieers @Akasurde I didn't want to update the "old" module vmware_dns_config because there were so many changes necessary anyway. How could we remove the old module since two modules with the same scope doesn't make sense?

@ansibot

This comment has been minimized.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Oct 18, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Oct 18, 2018
@dagwieers
Copy link
Contributor

dagwieers commented Oct 18, 2018

We cannot simply remove a module, we need to follow a deprecation process. This means that users will be notified that the module may disappear in 4 versions from now.

Another option is that we make the new module a drop-in replacement for the old one, by adding the parameters as aliases (or ghost parameters) so that they work exactly as before. In this case these ghost parameters or aliases could provide a deprecation-warning to the user so they have the time to modify their playbooks to the new design. (In this case we simply rename the old module to the new, so that the old name keeps on working as well)

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Oct 18, 2018
@ckotte ckotte force-pushed the vmware_host_dns branch 2 times, most recently from db80b7f to f4ccc15 Compare October 18, 2018 14:48
@ckotte
Copy link
Contributor Author

ckotte commented Oct 18, 2018

@dagwieers

We cannot simply remove a module, we need to follow a deprecation process. This means that users will be notified that the module may disappear in 4 versions from now.

Ok. I removed the old module, created a link, and some ghost parameters. Someone would just need to remove the code from the module and the symlink in 2.11, right?

@dagwieers
Copy link
Contributor

Yes, every time there's a new release, the code is being reviewed for deprecated stuff and cleaned up. That's why it's good to add a # NOTE: The below block is also deprecated starting from Ansible v2.11

@dagwieers
Copy link
Contributor

dagwieers commented Oct 18, 2018

@ckotte I don't see the aliases (or ghost parameters) for:

  • change_hostname_to
  • domainname

Also search_domains is now required, which will break any existing tasks not having this.
Which means your new module is not a drop-in replacement :-/

@ckotte ckotte force-pushed the vmware_host_dns branch 2 times, most recently from 1eb494e to 4333aa2 Compare October 28, 2018 15:32
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Nov 6, 2018
@ansibot ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Nov 25, 2018
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Dec 4, 2018
Copy link
Contributor

@pdellaert pdellaert left a comment

Choose a reason for hiding this comment

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

Great module, but have some questions on differences between module definition and documentation

requirements:
- python >= 2.6
- PyVmomi
options:
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the deprecated old change_hostname_to and domainname be mentioned as well? (i don't know the best protocol for ghost parameters)

['cluster_name', 'esxi_hostname'],
],
required_if=[
['type', 'static', ['host_name', 'domain', 'dns_servers', 'search_domains']],
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably good to add ['type', 'dhcp', ['device']]?

description:
- The hostname to be used for the ESXi host.
type: str
required: True
Copy link
Contributor

Choose a reason for hiding this comment

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

These required parameters seem not required if i look at the module definition in the code? Only if the type is static? Probably best to update documentation accordingly for host_name, domain, dns_servers and search_domains. Will probably require some changes in the ensure method as well

Or if they are required, the module definition should be updated.

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

ansibot commented Feb 19, 2019

@ansibot
Copy link
Contributor

ansibot commented Feb 27, 2019

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Mar 31, 2019
@ansibot
Copy link
Contributor

ansibot commented May 10, 2019

cc @goneri
click here for bot help


- debug: var=vcsim

- name: Wait for Flask controller to come up online
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

We recently did a refactoring of the functional test-suite, the goal is to be able to run it on a regular VMware environment, this instead of vcsim.
As a consequence, you should not handle vcsim in the functional test anymore, use the prepare_vmware_roles role instead. e.g: https://github.com/ansible/ansible/blob/devel/test/integration/targets/vmware_drs_group/tasks/main.yml

You should also ensure the resources that are created during your test are properly removed at the end.

@ansibot
Copy link
Contributor

ansibot commented May 31, 2019

@mariolenz
Copy link
Contributor

@ckotte I've cloned your feature branch and opened another PR: #64458

@Akasurde
Copy link
Member

superseded by #64458. Thanks @ckotte for contribution.

@Akasurde Akasurde closed this Nov 16, 2019
@ansible ansible locked and limited conversation to collaborators Dec 17, 2019
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 cloud deprecated This issue/PR relates to a deprecated module. has_issue 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_module This PR includes a new module. new_plugin This PR includes a new plugin. 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.

None yet

7 participants