Add options sub spec validation #27119

ganeshrn · 2017-07-20T15:33:00Z

SUMMARY

options handling will support checking each individual dict (element)
to resolve conditions for aliases, no_log, mutually_exclusive,
required, type check, value check, required_together, required_one_of
and required_if conditions in argspec. It will also set default values.

eg :

tasks:
  - name: Configure interface attribute with aggregate
    net_interface:
      aggregate:
        - {name: ge-0/0/1, description: test-interface-1, duplex: full, state: present}
        - {name: ge-0/0/2, description: test-interface-2, active: False}
      purge: Yes
    register: response

Usage:

   sub_argspec = dict(
        name=dict(),
        description=dict(no_log=True),
        duplex=dict(choices=['full', 'half', 'auto']),
        state=dict(default='present', choices=['present', 'absent', 'up', 'down']),
        active=dict(default=True, type='bool')
    )
    argument_spec = dict(
        aggregate=dict(type='list', elements='dict', options=sub_argspec),
    )

purge parameter in above example tasks will instruct the module to remove resources from the remote device that hasn’t been explicitly defined in aggregate. This is not supported by with_* iterators

Also, it improves performance as compared to with_* iterator for network device
that has a separate candidate and running datastore.

For with_* iteration, the sequence of operation is

load-config-1 (candidate db) -> commit (running db) -> load_config-2
(candidate db) -> commit (running db) ...

With aggregate the sequence of operation is

load-config-1 (candidate db) -> load-config-2 (candidate db) -> commit
(running db)

Since commit is executed once per task for aggregate it has
huge performance benefit for large configurations.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

module_utils/basic.py

ANSIBLE VERSION

2.4

ADDITIONAL INFORMATION

bcoca · 2017-07-20T17:34:35Z

nice, had 1/2 of this in patch, but have not had time to finish it, quick review looks good

the issue about implementation is that we had already agreed to use a ~~subspec~~(we changed several times, i still get confused) options key, which is already implemented by some modules and suported by the doc tools

ganeshrn · 2017-07-21T16:01:30Z

Looking at the current options handling in _check_argument_types() can we move the options check to __init__ and handle all options related things in _handle_options() function?
It will handle setting aliases, set defaults, no log values, check invalid arguments, argument type, value check and optional required_*, mutually_exclusive check if bypass_options_checks flag is false.

So aggregate in the network module spec will look like:

    argument_spec = dict(
        < -- snip -->
        aggregate=dict(type='list', elements='dict')
        < -- snip -->
    )

argument_spec['aggregate']['options'] = argument_spec

bcoca · 2017-07-21T16:11:45Z

yes to move options, no on having alternate implementation named 'aggregate', 'options' should be able to handle the requirements for network also

ganeshrn · 2017-07-23T12:46:56Z

bcoca: Added options validation and reverted aggregate implementation.

ganeshrn · 2017-07-24T06:11:25Z

ready_for_review

bcoca · 2017-07-24T13:27:16Z

cc @nitzmahone as he was also starting to develop this feature

bcoca · 2017-07-24T13:30:19Z

lib/ansible/module_utils/basic.py

@@ -773,7 +773,8 @@ class AnsibleModule(object):
    def __init__(self, argument_spec, bypass_checks=False, no_log=False,
                 check_invalid_arguments=True, mutually_exclusive=None, required_together=None,
                 required_one_of=None, add_file_common_args=False, supports_check_mode=False,
-                 required_if=None):
+                 required_if=None, bypass_options_checks=False, options_mutually_exclusive=None,
+                 options_required_together=None, options_required_one_of=None, options_required_if=None):


this seems to assume a single level of sub specs, in some cases we might want 2 or 3 so making it recursive with the top options seems like the best approach

For multi level sub specs if some of the args are same across different level it might result in a false error, that's why avoided having a recursive call.

making it recursive with the top options.

Does this mean required_* conditions or options_required_*?

bcoca: Fixed assuming module's top level options are applicable for multi-level sub args specs.

bcoca · 2017-07-24T13:31:39Z

lib/ansible/module_utils/basic.py


        return aliases_results

-    def _check_arguments(self, check_invalid_arguments):
+    def _handle_no_log_values(self, spec=None, param=None):


does not resolve inheritance #25097

Above referenced issue is not seen with this patch applied.
eg: Module arg spec

argument_spec = dict( name=dict(), description=dict(no_log=True), aggregate=dict(type='list', elements='dict'), purge=dict(default=False, type='bool'), state=dict(default='present', choices=['present', 'absent', 'up', 'down']), ) aggregate_arg_spec = deepcopy(argument_spec) del aggregate_arg_spec['aggregate'] argument_spec['aggregate']['options'] = aggregate_arg_spec

Module run log

"aggregate": [ { "description": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", "name": "ge-0/0/12", "purge": false, "state": "present", }, { "description": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", "name": "ge-0/0/13", "purge": false, "state": "present", }, { "description": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", "name": "ge-0/0/14", "purge": false, "state": "present", } ]

_handle_no_log_values() is also called from _handle_options() recursively in case on mullti-level argspec.
So if I understand your comment and referenced issue correctly, this patch does resolve inheritance issue.

aggregate parameter validation will support checking each individual dict to resolve conditions for aliases, no_log, mutually_exclusive, required, type check, values, required_together, required_one_of and required_if conditions in argspec. It will also set default values. eg: tasks: - name: Configure interface attribute with aggregate net_interface: aggregate: - {name: ge-0/0/1, description: test-interface-1, duplex: full, state: present} - {name: ge-0/0/2, description: test-interface-2, active: False} register: response purge: Yes Usage: ``` from ansible.module_utils.network_common import AggregateCollection transform = AggregateCollection(module) param = transform(module.params.get('aggregate')) ``` Aggregate allows supports for `purge` parameter, it will instruct the module to remove resources from remote device that hasn’t been explicitly defined in aggregate. This is not supported by with_* iterators Also, it improves performace as compared to with_* iterator for network device that has seperate candidate and running datastore. For with_* iteration the sequence of operartion is load-config-1 (candidate db) -> commit (running db) -> load_config-2 (candidate db) -> commit (running db) ... With aggregate the sequence of operation is load-config-1 (candidate db) -> load-config-2 (candidate db) -> commit (running db) As commit is executed only once per task for aggregate it has huge perfomance benefit for large configurations.

* Add support for options validation for aliases, no_log, mutually_exclusive, required, type check, value check, required_together, required_one_of and required_if conditions in sub-argspec. * Add unit test for options in argspec. * Reverted aggregate implementaion.

* Multi-level argspec support with module's top most conditionals options.

ansibot · 2017-07-28T07:23:59Z

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/module_utils/basic.py:1663:20: E225 missing whitespace around operator
lib/ansible/module_utils/basic.py:1766:20: E225 missing whitespace around operator

The test ansible-test sanity --test pylint failed with the following error:

lib/ansible/module_utils/network_common.py:176:12: undefined-variable Undefined variable 'module'

click here for bot help

ansibot · 2017-07-28T15:11:58Z

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/module_utils/basic.py:1663:20: E225 missing whitespace around operator
lib/ansible/module_utils/basic.py:1766:20: E225 missing whitespace around operator

click here for bot help

ganeshrn · 2017-07-28T16:59:27Z

lib/ansible/module_utils/basic.py

+                if spec is None or not params[k]:
+                    continue
+
+                self._options_context.append(k)


@nitzmahone Added ability to print parent context in case of error as discussed.

bcoca · 2017-08-01T12:37:38Z

lgtm, all issues I had have been addressed, I'm probably missing something but if we merge this soon we have plenty of time to debug/fix before release.

ganeshrn · 2017-08-01T15:40:38Z

bcoca: So shall I go ahead and merge this PR?

* Add aggregate parameter validation aggregate parameter validation will support checking each individual dict to resolve conditions for aliases, no_log, mutually_exclusive, required, type check, values, required_together, required_one_of and required_if conditions in argspec. It will also set default values. eg: tasks: - name: Configure interface attribute with aggregate net_interface: aggregate: - {name: ge-0/0/1, description: test-interface-1, duplex: full, state: present} - {name: ge-0/0/2, description: test-interface-2, active: False} register: response purge: Yes Usage: ``` from ansible.module_utils.network_common import AggregateCollection transform = AggregateCollection(module) param = transform(module.params.get('aggregate')) ``` Aggregate allows supports for `purge` parameter, it will instruct the module to remove resources from remote device that hasn’t been explicitly defined in aggregate. This is not supported by with_* iterators Also, it improves performace as compared to with_* iterator for network device that has seperate candidate and running datastore. For with_* iteration the sequence of operartion is load-config-1 (candidate db) -> commit (running db) -> load_config-2 (candidate db) -> commit (running db) ... With aggregate the sequence of operation is load-config-1 (candidate db) -> load-config-2 (candidate db) -> commit (running db) As commit is executed only once per task for aggregate it has huge perfomance benefit for large configurations. * Fix CI issues * Fix review comments * Add support for options validation for aliases, no_log, mutually_exclusive, required, type check, value check, required_together, required_one_of and required_if conditions in sub-argspec. * Add unit test for options in argspec. * Reverted aggregate implementaion. * Minor change * Add multi-level argspec support * Multi-level argspec support with module's top most conditionals options. * Fix unit test failure * Add parent context in errors for sub options * Resolve merge conflict * Fix CI issue

caphrim007 · 2017-08-09T18:57:59Z

@ganeshrn this broke fallback functionality in the f5_utils file. Was this supposed to happen?

nitzmahone · 2017-08-10T00:28:28Z

lib/ansible/module_utils/basic.py

+
+                        self._check_required_together(self.required_together, param)
+                        self._check_required_one_of(self.required_one_of, param)
+                        self._check_required_if(self.required_if, param)


Upon further reflection, I think these three lines along with check_mutually_exclusive are erroneous for subspecs. We don't currently have a way to specify required_if/one_of/together or mutually_exclusive for subspecs, so in every case but the networking "aggregate" thing, this is severely broken (as you'd be making nonsensical checks in most cases)

Only sane way I could think of to support this properly would be to make those values reserved arg names in the arg_spec (so we could define them at any layer of the dict instead of passing them alongside). @bcoca: any other ideas?

ganeshrn requested review from privateip and bcoca July 20, 2017 15:33

bcoca removed the needs_triage Needs a first human triage before being processed. label Jul 20, 2017

bcoca mentioned this pull request Jul 21, 2017

subspec defaults support #25226

Closed

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 21, 2017

ganeshrn changed the title ~~Add aggregate parameter validation~~ Add options sub spec validation Jul 23, 2017

bcoca reviewed Jul 24, 2017

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 25, 2017

ganeshrn added 7 commits July 28, 2017 11:59

Fix CI issues

31c6b8b

Fix review comments

ade4a6b

* Add support for options validation for aliases, no_log, mutually_exclusive, required, type check, value check, required_together, required_one_of and required_if conditions in sub-argspec. * Add unit test for options in argspec. * Reverted aggregate implementaion.

Minor change

a8b4fb1

Add multi-level argspec support

d19519d

* Multi-level argspec support with module's top most conditionals options.

Fix unit test failure

d1eef33

Add parent context in errors for sub options

50ab0ac

ganeshrn force-pushed the aggregate_validate branch from 2c4177f to 50ab0ac Compare July 28, 2017 06:37

ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 28, 2017

Resolve merge conflict

c06af97

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 28, 2017

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 28, 2017

Fix CI issue

b3e0838

ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 28, 2017

ganeshrn commented Jul 28, 2017

View reviewed changes

ganeshrn added this to the 2.4.0 milestone Jul 28, 2017

nitzmahone merged commit 97a34cf into ansible:devel Aug 1, 2017

ganeshrn deleted the aggregate_validate branch August 1, 2017 16:46

caphrim007 mentioned this pull request Aug 9, 2017

Address breaking PR for fallbacks F5Networks/f5-ansible#303

Closed

nitzmahone reviewed Aug 10, 2017

View reviewed changes

ganeshrn mentioned this pull request Aug 29, 2017

argument_spec: no_log does not hide sub options #25097

Closed

Qalthos mentioned this pull request Aug 31, 2017

nxos_config logs your password to syslog in plaintext #28724

Closed

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 5, 2018

Akasurde mentioned this pull request Jun 3, 2018

New module: xenserver_guest - manages XenServer VMs #41041

Merged

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add options sub spec validation #27119

Add options sub spec validation #27119

ganeshrn commented Jul 20, 2017 •

edited

bcoca commented Jul 20, 2017 •

edited

ganeshrn commented Jul 21, 2017

bcoca commented Jul 21, 2017

ganeshrn commented Jul 23, 2017

ganeshrn commented Jul 24, 2017

bcoca commented Jul 24, 2017

bcoca Jul 24, 2017

ganeshrn Jul 24, 2017

ganeshrn Jul 24, 2017 •

edited

ganeshrn Jul 25, 2017

bcoca Jul 24, 2017

ganeshrn Jul 24, 2017

ganeshrn Jul 25, 2017

ansibot commented Jul 28, 2017

ansibot commented Jul 28, 2017

ganeshrn Jul 28, 2017

bcoca commented Aug 1, 2017

ganeshrn commented Aug 1, 2017

caphrim007 commented Aug 9, 2017

nitzmahone Aug 10, 2017 •

edited

nitzmahone Aug 10, 2017

Add options sub spec validation #27119

Add options sub spec validation #27119

Conversation

ganeshrn commented Jul 20, 2017 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

bcoca commented Jul 20, 2017 • edited

ganeshrn commented Jul 21, 2017

bcoca commented Jul 21, 2017

ganeshrn commented Jul 23, 2017

ganeshrn commented Jul 24, 2017

bcoca commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganeshrn Jul 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Jul 28, 2017

ansibot commented Jul 28, 2017

Choose a reason for hiding this comment

bcoca commented Aug 1, 2017

ganeshrn commented Aug 1, 2017

caphrim007 commented Aug 9, 2017

nitzmahone Aug 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganeshrn commented Jul 20, 2017 •

edited

bcoca commented Jul 20, 2017 •

edited

ganeshrn Jul 24, 2017 •

edited

nitzmahone Aug 10, 2017 •

edited