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: fix customization.hostname handling #52779

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@dgmorales
Copy link

dgmorales commented Feb 22, 2019

SUMMARY

Stops silently mangling of the user supplied customization.hostname to fit VMware API requirements (unless --force is set). Instead it throws a helpful error message if the name does not fit the requirements.

This PR is an alternative proposal to #46056.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/modules/cloud/vmware/vmware_guest.py

ADDITIONAL INFORMATION

This is related to a discussion that started in #38005 (comment) and was followed up on PR #46056.

In the code proposed here:

  • customization.hostname is not required, get it's default from vm name
  • if force==False, no silently hostname mangling is done. If name is not valid, a helpful message is throw
  • Also checks that the name contains at least one letter, as said in the vmware doc.
  • if force==True, silently mangle the name or customization.hostname as currently done (unless name has no letter)

I didn't test it against a VMware enviroment yet. I will do it soon on the following days. But I did try to test the logic mocking an object with a params attribute. Here's the results of my testing:

(first hostname is vm name, followed by FAILED or final hostname)

With customization.hostname unset and force==False:

hostname "vmware01.example.com" ... FAILED
hostname "host.d" ... FAILED
hostname "a xyz" ... FAILED
hostname "90921" ... FAILED
hostname "-0" ... FAILED
hostname "a" ... a
hostname "A" ... A
hostname "xyz-aa-0" ... xyz-aa-0
hostname "2a131" ... 2a131
hostname "vmware01" ... vmware01
hostname "Vmware" ... Vmware

With customization.hostname unset and force==True

hostname "vmware01.example.com" ... vmware01
hostname "host.d" ... host
hostname "a xyz" ... axyz
hostname "90921" ... FAILED
hostname "-0" ... FAILED
hostname "a" ... a
hostname "A" ... A
hostname "xyz-aa-0" ... xyz-aa-0
hostname "2a131" ... 2a131
hostname "vmware01" ... vmware01
hostname "Vmware" ... Vmware

Also:

  • Setting customization.hostname to "vmware-01.example.com" and Force==False fails every case
  • Setting customization.hostname to "vmware-01.example.com" and Force==True returns "vmware-01" to every case
@dgmorales

This comment has been minimized.

Copy link
Author

dgmorales commented Feb 22, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 22, 2019

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
cat _themes/srtd/static/css/theme.css | sed -e 's/^[ 	]*//g; s/[ 	]*$//g; s/\([:{;,]\) /\1/g; s/ {/{/g; s/\/\*.*\*\///g; /^$/d' | sed -e :a -e '$!N; s/\n\(.\)/\1/; ta' > _themes/srtd/static/css/theme.min.css
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
PYTHONPATH=../../lib ../bin/dump_keywords.py --template-dir=../templates --output-dir=rst/reference_appendices/ -d ./keyword_desc.yml
PYTHONPATH=../../lib ../bin/plugin_formatter.py -t rst --template-dir=../templates --module-dir=../../lib/ansible/modules -o rst/modules/ 
Evaluating module files...
Makefile:93: recipe for target 'modules' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/plugin_formatter.py", line 774, in <module>
    main()
  File "../bin/plugin_formatter.py", line 729, in main
    plugin_info, categories = get_plugin_info(options.module_dir, limit_to=options.limit_to, verbose=(options.verbosity > 0))
  File "../bin/plugin_formatter.py", line 294, in get_plugin_info
    doc, examples, returndocs, metadata = plugin_docs.get_docstring(module_path, fragment_loader, verbose=verbose)
  File "/root/ansible/lib/ansible/utils/plugin_docs.py", line 103, in get_docstring
    data = read_docstring(filename, verbose=verbose, ignore_errors=ignore_errors)
  File "/root/ansible/lib/ansible/parsing/plugin_docs.py", line 59, in read_docstring
    data[varkey] = AnsibleLoader(child.value.s, file_name=filename).get_single_data()
  File "/usr/local/lib/python3.6/dist-packages/yaml/constructor.py", line 35, in get_single_data
    node = self.get_single_node()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 36, in get_single_node
    document = self.compose_document()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 55, in compose_document
    node = self.compose_node(None, None)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 127, in compose_mapping_node
    while not self.check_event(MappingEndEvent):
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 98, in check_event
    self.current_event = self.state()
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 439, in parse_block_mapping_key
    "expected <block end>, but found %r" % token.id, token.start_mark)
yaml.parser.ParserError: while parsing a block mapping
  in "<unicode string>", line 288, column 5:
        description:
        ^
expected <block end>, but found '<scalar>'
  in "<unicode string>", line 304, column 8:
        - 'Parameters related to Windows cu ... 
           ^
make: *** [modules] Error 1

The test ansible-test sanity --test validate-modules [explain] failed with 10 errors:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'datacenter' in argument_spec defines default as ('ha-datacenter') but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'name_match' in argument_spec defines default as ('first') but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'port' in argument_spec defines default as (443) but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'state' in argument_spec defines default as ('present') but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'validate_certs' in argument_spec defines default as (True) but documentation defines default as (False)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E326 Argument 'convert' in argument_spec defines choices as (['thin', 'thick', 'eagerzeroedthick']) but documentation defines choices as ([])
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E326 Argument 'name_match' in argument_spec defines choices as (['first', 'last']) but documentation defines choices as ([])
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E326 Argument 'state' in argument_spec defines choices as (['absent', 'poweredoff', 'poweredon', 'present', 'rebootguest', 'restarted', 'shutdownguest', 'suspended']) but documentation defines choices as ([])
lib/ansible/modules/cloud/vmware/vmware_guest.py:317:8: E302 DOCUMENTATION is not valid YAML
test/sanity/validate-modules/ignore.txt:336:1: A102 Remove since "lib/ansible/modules/cloud/vmware/vmware_guest.py" passes "E322" test

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

lib/ansible/modules/cloud/vmware/vmware_guest.py:317:8: error DOCUMENTATION: syntax error: expected <block end>, but found '<scalar>'

click here for bot help

@ansibot

This comment has been minimized.

@dgmorales

This comment has been minimized.

Copy link
Author

dgmorales commented Feb 22, 2019

I see there are some errors detected by the linter, will work on that.

@ansibot ansibot removed the needs_triage label Feb 22, 2019

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Feb 22, 2019

@pdellaert @dericcrago @Im0 @ckotte @jeking3 Could you please review this ? Thanks in advance.

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Feb 22, 2019

Rest LGTM

- ' - C(hostname) (string): Computer hostname (default: shorted C(name) parameter). Allowed characters are alphanumeric (uppercase and lowercase)
and minus, rest of the characters are dropped as per RFC 952.'
- ' - C(hostname) (string): Computer hostname (default: C(name) parameter). Allowed characters are alphanumeric (uppercase and lowercase)
and minus (no dots allowed, VMware guest customization for linux does not support receiving a FQDN name). Use "force=True" for old

This comment has been minimized.

@mattclay

mattclay Feb 22, 2019

Member

Making the new behavior the default is not backwards compatible. The new behavior should be opt-in rather than opt-out to avoid causing problems with existing playbooks during an upgrade.

This comment has been minimized.

@dgmorales

dgmorales Feb 23, 2019

Author

An important general rule indeed, but IMHO this should be an exception.

Because I believe the current behaviour is misleading. Not sure if it's the best word to describe, but It made me waste time figuring out why the domain name was not being configured in the OS hostname as I had instructed the module to do. Instead it could just have thrown an error telling me it wasn't supported. Can happen to many others, probably have already.

I also believe that the likeliness of someone out there relying on this behaviour is not that big. It could surely be masking an configuration error that will be revealed with this, but should be an easy fix, and people should validate/test new ansible versions before adopting then anyway.

Could you give use some advice on that @Akasurde ?

If we make the new behaviour opt-in, then it would need to be something more than just trigger the new behaviour with --force, because that would be also confusing. Any suggestions in that case?

@ansibot ansibot removed the ci_verified label Feb 23, 2019

@dgmorales

This comment has been minimized.

Copy link
Author

dgmorales commented Feb 23, 2019

Well, now it's throwing an error about the hostname given in one of the tests. I'll check it out and propose a fix for the test, if that's the case.

When done should I squash my commits and push force, or leave the squashing for merge time?

@ansibot ansibot added the stale_ci label Mar 5, 2019

@dgmorales dgmorales force-pushed the stone-payments:fix/stop-mangling-customization-hostname branch from 02577eb to 2a780fa Mar 6, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 6, 2019

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
PYTHONPATH=../../lib ../bin/dump_keywords.py --template-dir=../templates --output-dir=rst/reference_appendices/ -d ./keyword_desc.yml
PYTHONPATH=../../lib ../bin/plugin_formatter.py -t rst --template-dir=../templates --module-dir=../../lib/ansible/modules -o rst/modules/ 
Evaluating module files...
Makefile:93: recipe for target 'modules' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/plugin_formatter.py", line 774, in <module>
    main()
  File "../bin/plugin_formatter.py", line 729, in main
    plugin_info, categories = get_plugin_info(options.module_dir, limit_to=options.limit_to, verbose=(options.verbosity > 0))
  File "../bin/plugin_formatter.py", line 294, in get_plugin_info
    doc, examples, returndocs, metadata = plugin_docs.get_docstring(module_path, fragment_loader, verbose=verbose)
  File "/root/ansible/lib/ansible/utils/plugin_docs.py", line 105, in get_docstring
    data = read_docstring(filename, verbose=verbose, ignore_errors=ignore_errors)
  File "/root/ansible/lib/ansible/parsing/plugin_docs.py", line 59, in read_docstring
    data[varkey] = AnsibleLoader(child.value.s, file_name=filename).get_single_data()
  File "/usr/local/lib/python3.6/dist-packages/yaml/constructor.py", line 35, in get_single_data
    node = self.get_single_node()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 36, in get_single_node
    document = self.compose_document()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 55, in compose_document
    node = self.compose_node(None, None)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 127, in compose_mapping_node
    while not self.check_event(MappingEndEvent):
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 98, in check_event
    self.current_event = self.state()
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 439, in parse_block_mapping_key
    "expected <block end>, but found %r" % token.id, token.start_mark)
yaml.parser.ParserError: while parsing a block mapping
  in "<unicode string>", line 294, column 5:
        description:
        ^
expected <block end>, but found '<scalar>'
  in "<unicode string>", line 310, column 8:
        - 'Parameters related to Windows cu ... 
           ^
make: *** [modules] Error 1

The test ansible-test sanity --test validate-modules [explain] failed with 10 errors:

lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'datacenter' in argument_spec defines default as ('ha-datacenter') but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'name_match' in argument_spec defines default as ('first') but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'port' in argument_spec defines default as (443) but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'state' in argument_spec defines default as ('present') but documentation defines default as (None)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E324 Argument 'validate_certs' in argument_spec defines default as (True) but documentation defines default as (False)
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E326 Argument 'convert' in argument_spec defines choices as (['thin', 'thick', 'eagerzeroedthick']) but documentation defines choices as ([])
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E326 Argument 'name_match' in argument_spec defines choices as (['first', 'last']) but documentation defines choices as ([])
lib/ansible/modules/cloud/vmware/vmware_guest.py:0:0: E326 Argument 'state' in argument_spec defines choices as (['absent', 'poweredoff', 'poweredon', 'present', 'rebootguest', 'restarted', 'shutdownguest', 'suspended']) but documentation defines choices as ([])
lib/ansible/modules/cloud/vmware/vmware_guest.py:323:8: E302 DOCUMENTATION is not valid YAML
test/sanity/validate-modules/ignore.txt:332:1: A102 Remove since "lib/ansible/modules/cloud/vmware/vmware_guest.py" passes "E322" test

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

lib/ansible/modules/cloud/vmware/vmware_guest.py:323:8: error DOCUMENTATION: syntax error: expected <block end>, but found '<scalar>'

click here for bot help

@mattclay mattclay added the ci_verified label Mar 6, 2019

VMware: fix customization.hostname handling
- Stops silently mangling of the user supplied customization.hostname to
  fit VMware API requirements (unless --force is set). Instead it throws a
  helpful error message if the name does not fit the requirements.
- Fix existing test case with VM name that does not fit the
  requirements.
- Add a new test case for customization hostname validation

@dgmorales dgmorales force-pushed the stone-payments:fix/stop-mangling-customization-hostname branch from 2a780fa to bb91bde Mar 8, 2019

@ansibot ansibot removed the ci_verified label Mar 8, 2019

@dgmorales

This comment has been minimized.

Copy link
Author

dgmorales commented Mar 8, 2019

I have:

  • Fixed a test case that used a name that didn't fit there requirements of the VMware API
  • Added a test case for customisation hostname validation
  • Squashed my commits and rebased to current devel
  • Fixed another syntax error detected by the CI

I've ran some ansible-test sanity tests locally and it went fine. I'm waiting on shippable CI, hope it passes now.

@dgmorales

This comment has been minimized.

Copy link
Author

dgmorales commented Mar 8, 2019

While I was doing that I've began to question if the same approach should be applied to the windows customisation code, since this is just for linux, but the docs says the requirements are the same (see this doc). The windows code is very different and I'm not sure if it's a good idea to consider changing that in this same PR.

@ansibot ansibot added the stale_ci label Mar 21, 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.