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

WIP: Add apply to k8s module #49053

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@willthames
Contributor

willthames commented Nov 23, 2018

SUMMARY

Use apply method for updating k8s resources.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

k8s

@willthames

This comment has been minimized.

Contributor

willthames commented Nov 23, 2018

Relies on as-yet unreleased openshift-restclient-python functionality - this is effectively a companion test suite and the prototype for what it will look like from the Ansible side.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 23, 2018

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

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 23, 2018

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

lib/ansible/modules/clustering/k8s/k8s.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/clustering/k8s/k8s.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/clustering/k8s/k8s.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/clustering/k8s/k8s.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/clustering/k8s/k8s.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/ 
Makefile:93: recipe for target 'modules' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/plugin_formatter.py", line 720, in <module>
    main()
  File "../bin/plugin_formatter.py", line 678, 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 269, 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 96, 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 88, column 5:
        description:
        ^
expected <block end>, but found '<scalar>'
  in "<unicode string>", line 90, column 15:
        - 'apply' remembers previous invocations o ... 
                  ^
make: *** [modules] Error 1

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

lib/ansible/module_utils/k8s/raw.py:331:13: E303 too many blank lines (2)

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

lib/ansible/modules/clustering/k8s/k8s.py:0:0: E324 Value for "default" from the argument_spec ('present') for "state" does not match the documentation (None)
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E324 Value for "default" from the argument_spec ('v1') for "api_version" does not match the documentation (None)
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E324 Value for "default" from the argument_spec (120) for "wait_timeout" does not match the documentation (None)
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E325 argument_spec for "append_hash" defines type="bool" but documentation does not
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E325 argument_spec for "apply" defines type="bool" but documentation does not
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E325 argument_spec for "force" defines type="bool" but documentation does not
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E325 argument_spec for "verify_ssl" defines type="bool" but documentation does not
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E325 argument_spec for "wait" defines type="bool" but documentation does not
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E326 Value for "choices" from the argument_spec (['present', 'absent']) for "state" does not match the documentation ([])
lib/ansible/modules/clustering/k8s/k8s.py:0:0: E326 Value for "choices" from the argument_spec ([['json'], ['merge'], ['strategic-merge']]) for "merge_type" does not match the documentation ([])
lib/ansible/modules/clustering/k8s/k8s.py:105:15: E302 DOCUMENTATION is not valid YAML

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

lib/ansible/modules/clustering/k8s/k8s.py:105:15: error DOCUMENTATION: syntax error: expected <block end>, but found '<scalar>'

click here for bot help

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 24, 2018

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

lib/ansible/module_utils/k8s/raw.py:331:13: E303 too many blank lines (2)

click here for bot help

@willthames willthames force-pushed the willthames:k8s_apply branch from 0fdd07f to 19f3d2a Nov 24, 2018

@fabianvf

Looks really good. One more thing to note, openshift is dropping the start api subcommand that we're using to bring it up for testing, so we may need to find a different way to start the test environment soon. I'm looking into a few things ATM. Not relevant to this PR yet but wanted to mention.

@@ -51,6 +49,12 @@
except ImportError:
HAS_K8S_CONFIG_HASH = False
try:
from openshift.dynamic.apply import apply as k8s_apply

This comment has been minimized.

@fabianvf

fabianvf Nov 25, 2018

Contributor

In the actual implementation I think apply will be another method on the client object, just like patch/replace etc (though a lot of the logic will likely stay in this module)

This comment has been minimized.

@willthames

willthames Nov 25, 2018

Contributor

No worries, this PR will remain a WIP until the openshift implementation is finalised

description:
- Whether to use 'apply' method to update resources.
- "'apply' remembers previous invocations of k8s apply through an annotation"
- "'apply' attempts to ensure a resource is exactly as specified - it's closer to a replace than a patch."

This comment has been minimized.

@fabianvf

fabianvf Nov 25, 2018

Contributor

I don't think I'd say this, it's more like a really smart patch that can automatically compute deletions. It's still going to be sending a PATCH and not a PUT so I'd just worry someone gets the wrong idea.

This comment has been minimized.

@willthames

willthames Nov 25, 2018

Contributor

Thanks, I'll rework the wording

@@ -29,8 +29,6 @@
from ansible.module_utils.k8s.common import KubernetesAnsibleModule
from ansible.module_utils.common.dict_transformations import dict_merge
from distutils.version import LooseVersion

This comment has been minimized.

@fabianvf

fabianvf Nov 25, 2018

Contributor

Do we not need this? It looks like it's still used below

This comment has been minimized.

@willthames

willthames Nov 25, 2018

Contributor

See line 23

if not HAS_K8S_CONFIG_HASH:
self.fail_json(msg="openshift >= 0.7.2 is required for append_hash")
if self.append_hash and not HAS_K8S_CONFIG_HASH:
self.fail_json(msg="openshift >= 0.7.2 is required for append_hash")
if self.params['merge_type']:

This comment has been minimized.

@fabianvf

fabianvf Nov 25, 2018

Contributor

One thing to note, merge_type might not work with apply, we might hard code the apply merge strategy to merge (since strategic merge patch would need to know the merge key to intelligently generate the patch)

This comment has been minimized.

@willthames

willthames Nov 25, 2018

Contributor

Yes, this is true, I was thinking that merge_type and apply should be mutually exclusive, I just clearly forgot to actually do that!

This comment has been minimized.

@willthames

willthames Nov 25, 2018

Contributor

I should also make sure that the crd test suite works as well with apply as it does with merge_yaml

@@ -71,6 +75,7 @@ def argspec(self):
argument_spec['wait_timeout'] = dict(type='int', default=120)
argument_spec['validate'] = dict(type='dict', default=None, options=self.validate_spec)
argument_spec['append_hash'] = dict(type='bool', default=False)
argument_spec['apply'] = dict(type='bool', default=False)

This comment has been minimized.

@fabianvf

fabianvf Nov 25, 2018

Contributor

I understand defaulting to false for the sake of an unsurprising update, but should we consider making apply the default (in this or in a later version) since it will likely be so much less error prone?

This comment has been minimized.

@willthames

willthames Nov 25, 2018

Contributor

I agree with you, I'm just not sure how to do this in an unsurprising backward compatible way.

This comment has been minimized.

@fabianvf

fabianvf Nov 26, 2018

Contributor

Well, backwards compatibility should remain (using apply vs the old way shouldn't really change anything, other than decreasing the number of potential errors), the only real difference between the old way and apply should be that an annotation appears on your object. It might be enough to mention this in the porting guide and just rip the bandaid off, rather than defaulting to an inferior implementation. I'm not sure though, right now it just feels more wrong to add apply and not set it as default than it does to make the change.

@ansibot ansibot removed the needs_triage label Nov 25, 2018

willthames added some commits Nov 24, 2018

Add apply to k8s module
Use apply method for updating k8s resources.

@willthames willthames force-pushed the willthames:k8s_apply branch from 19f3d2a to 2e9e48f Nov 26, 2018

@ansibot ansibot added the stale_ci label Dec 4, 2018

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