From 6eecbf10e6464dc81bc0175d70cd497de50b16e9 Mon Sep 17 00:00:00 2001 From: saichint Date: Mon, 23 Apr 2018 08:08:49 -0700 Subject: [PATCH] fix nxos_igmp_interface issues (#38752) * fix nxos_igmp_interface issues * shippable fix * fix oif_prefix and oif_source * shippable fix * shippable fix * shippable fix * add an example for oif_ps * review comments * review comments * more review comments * fix typo --- CHANGELOG.md | 2 + .../rst/porting_guides/porting_guide_2.6.rst | 3 +- .../network/nxos/nxos_igmp_interface.py | 239 +++++++++--------- .../tests/common/sanity.yaml | 110 +++++++- 4 files changed, 231 insertions(+), 123 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9673479ffb9..2ec384455c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ Ansible Changes By Release ### Major Changes ### Deprecations (to be removed in 2.10) +* In the `nxos_igmp_interface` module, `oif_prefix` and `oif_source` properties are deprecated. + Use the `oif_ps` parameter with a dictionary of prefix and source to values instead. See [Porting Guide](http://docs.ansible.com/ansible/devel/porting_guides/porting_guides.html) for more information diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.6.rst b/docs/docsite/rst/porting_guides/porting_guide_2.6.rst index ee9b8ac401b..2bc96fad434 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_2.6.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_2.6.rst @@ -22,7 +22,8 @@ No notable changes. Deprecated ========== -No notable changes. +* In the :ref:`nxos_igmp_interface ` module, `oif_prefix` and `oif_source` properties are deprecated. Use + `ois_ps` parameter with a dictionary of prefix and source to values instead. Modules ======= diff --git a/lib/ansible/modules/network/nxos/nxos_igmp_interface.py b/lib/ansible/modules/network/nxos/nxos_igmp_interface.py index 300fabe4de7..712ad2e2b56 100644 --- a/lib/ansible/modules/network/nxos/nxos_igmp_interface.py +++ b/lib/ansible/modules/network/nxos/nxos_igmp_interface.py @@ -39,14 +39,15 @@ notes: C(startup_query_count), C(robustness), C(querier_timeout), C(query_mrt), C(query_interval), C(last_member_qrt), C(last_member_query_count), C(group_timeout), C(report_llg), and C(immediate_leave). - - When C(state=absent), all configs for C(oif_prefix), C(oif_source), and + - When C(state=absent), all configs for C(oif_ps), and C(oif_routemap) will be removed. - PIM must be enabled to use this module. - This module is for Layer 3 interfaces. - Route-map check not performed (same as CLI) check when configuring route-map with 'static-oif' - If restart is set to true with other params set, the restart will happen - last, i.e. after the configuration takes place. + last, i.e. after the configuration takes place. However, 'restart' itself + is not idempotent as it is an action and not configuration. options: interface: description: @@ -55,48 +56,51 @@ options: required: true version: description: - - IGMP version. It can be 2 or 3. - choices: ['2', '3'] + - IGMP version. It can be 2 or 3 or keyword 'default'. + choices: ['2', '3', 'default'] startup_query_interval: description: - Query interval used when the IGMP process starts up. - The range is from 1 to 18000. The default is 31. + The range is from 1 to 18000 or keyword 'default'. + The default is 31. startup_query_count: description: - Query count used when the IGMP process starts up. - The range is from 1 to 10. The default is 2. + The range is from 1 to 10 or keyword 'default'. + The default is 2. robustness: description: - - Sets the robustness variable. Values can range from 1 to 7. - The default is 2. + - Sets the robustness variable. Values can range from 1 to 7 or + keyword 'default'. The default is 2. querier_timeout: description: - Sets the querier timeout that the software uses when deciding to take over as the querier. Values can range from 1 to 65535 - seconds. The default is 255 seconds. + seconds or keyword 'default'. The default is 255 seconds. query_mrt: description: - Sets the response time advertised in IGMP queries. - Values can range from 1 to 25 seconds. The default is 10 seconds. + Values can range from 1 to 25 seconds or keyword 'default'. + The default is 10 seconds. query_interval: description: - Sets the frequency at which the software sends IGMP host query - messages. Values can range from 1 to 18000 seconds. - The default is 125 seconds. + messages. Values can range from 1 to 18000 seconds or keyword + 'default'. The default is 125 seconds. last_member_qrt: description: - Sets the query interval waited after sending membership reports before the software deletes the group state. Values can range - from 1 to 25 seconds. The default is 1 second. + from 1 to 25 seconds or keyword 'default'. The default is 1 second. last_member_query_count: description: - Sets the number of times that the software sends an IGMP query in response to a host leave message. - Values can range from 1 to 5. The default is 2. + Values can range from 1 to 5 or keyword 'default'. The default is 2. group_timeout: description: - Sets the group membership timeout for IGMPv2. - Values can range from 3 to 65,535 seconds. + Values can range from 3 to 65,535 seconds or keyword 'default'. The default is 260 seconds. report_llg: description: @@ -105,7 +109,6 @@ options: Reports are always sent for nonlink local groups. By default, reports are not sent for link local groups. type: bool - default: 'no' immediate_leave: description: - Enables the device to remove the group entry from the multicast @@ -115,30 +118,44 @@ options: device does not send group-specific queries. The default is disabled. type: bool - default: 'no' oif_routemap: description: - - Configure a routemap for static outgoing interface (OIF). + - Configure a routemap for static outgoing interface (OIF) or + keyword 'default'. oif_prefix: description: - - Configure a prefix for static outgoing interface (OIF). + - This argument is deprecated, please use oif_ps instead. + Configure a prefix for static outgoing interface (OIF). oif_source: description: - - Configure a source for static outgoing interface (OIF). + - This argument is deprecated, please use oif_ps instead. + Configure a source for static outgoing interface (OIF). + oif_ps: + description: + - Configure prefixes and sources for static outgoing interface (OIF). This + is a list of dict where each dict has source and prefix defined or just + prefix if source is not needed. The specified values will be configured + on the device and if any previous prefix/sources exist, they will be removed. + Keyword 'default' is also accpted which removes all existing prefix/sources. + version_added: 2.6 restart: description: - - Restart IGMP. + - Restart IGMP. This is NOT idempotent as this is action only. type: bool + default: False state: description: - Manages desired state of the resource. default: present - choices: ['present', 'default'] + choices: ['present', 'absent', 'default'] ''' EXAMPLES = ''' - nxos_igmp_interface: interface: ethernet1/32 startup_query_interval: 30 + oif_ps: + - { 'prefix': '238.2.2.6' } + - { 'source': '192.168.0.1', 'prefix': '238.2.2.5'} state: present ''' RETURN = ''' @@ -146,54 +163,25 @@ proposed: description: k/v pairs of parameters passed into module returned: always type: dict - sample: {"asn": "65535", "router_id": "1.1.1.1", "vrf": "test"} + sample: {"startup_query_count": "30", + "oif_ps": [{'prefix': '238.2.2.6'}, {'source': '192.168.0.1', 'prefix': '238.2.2.5'}]} existing: - description: k/v pairs of existing BGP configuration + description: k/v pairs of existing igmp_interface configuration returned: always type: dict - sample: {"asn": "65535", "bestpath_always_compare_med": false, - "bestpath_aspath_multipath_relax": false, - "bestpath_compare_neighborid": false, - "bestpath_compare_routerid": false, - "bestpath_cost_community_ignore": false, - "bestpath_med_confed": false, - "bestpath_med_missing_as_worst": false, - "bestpath_med_non_deterministic": false, "cluster_id": "", - "confederation_id": "", "confederation_peers": "", - "graceful_restart": true, "graceful_restart_helper": false, - "graceful_restart_timers_restart": "120", - "graceful_restart_timers_stalepath_time": "300", "local_as": "", - "log_neighbor_changes": false, "maxas_limit": "", - "neighbor_down_fib_accelerate": false, "reconnect_interval": "60", - "router_id": "11.11.11.11", "suppress_fib_pending": false, - "timer_bestpath_limit": "", "timer_bgp_hold": "180", - "timer_bgp_keepalive": "60", "vrf": "test"} + sample: {"startup_query_count": "2", "oif_ps": []} end_state: - description: k/v pairs of BGP configuration after module execution + description: k/v pairs of igmp interface configuration after module execution returned: always type: dict - sample: {"asn": "65535", "bestpath_always_compare_med": false, - "bestpath_aspath_multipath_relax": false, - "bestpath_compare_neighborid": false, - "bestpath_compare_routerid": false, - "bestpath_cost_community_ignore": false, - "bestpath_med_confed": false, - "bestpath_med_missing_as_worst": false, - "bestpath_med_non_deterministic": false, "cluster_id": "", - "confederation_id": "", "confederation_peers": "", - "graceful_restart": true, "graceful_restart_helper": false, - "graceful_restart_timers_restart": "120", - "graceful_restart_timers_stalepath_time": "300", "local_as": "", - "log_neighbor_changes": false, "maxas_limit": "", - "neighbor_down_fib_accelerate": false, "reconnect_interval": "60", - "router_id": "1.1.1.1", "suppress_fib_pending": false, - "timer_bestpath_limit": "", "timer_bgp_hold": "180", - "timer_bgp_keepalive": "60", "vrf": "test"} + sample: {"startup_query_count": "30", + "oif_ps": [{'prefix': '238.2.2.6'}, {'source': '192.168.0.1', 'prefix': '238.2.2.5'}]} updates: description: commands sent to the device returned: always type: list - sample: ["router bgp 65535", "vrf test", "router-id 1.1.1.1"] + sample: ["interface Ethernet1/32", "ip igmp startup-query-count 30", + "ip igmp static-oif 238.2.2.6", "ip igmp static-oif 238.2.2.5 source 192.168.0.1"] changed: description: check to see if a change was made on the device returned: always @@ -299,6 +287,8 @@ def get_igmp_interface(module, interface): body = execute_show_command(command, module)[0] if body: + if 'not running' in body: + return igmp resource = body['TABLE_vrf']['ROW_vrf']['TABLE_if']['ROW_if'] igmp = apply_key_map(key_map, resource) report_llg = str(resource['ReportingForLinkLocal']).lower() @@ -369,7 +359,7 @@ def get_igmp_interface(module, interface): return igmp -def config_igmp_interface(delta, found_both, found_prefix): +def config_igmp_interface(delta, existing, existing_oif_prefix_source): CMDS = { 'version': 'ip igmp version {0}', 'startup_query_interval': 'ip igmp startup-query-interval {0}', @@ -390,19 +380,46 @@ def config_igmp_interface(delta, found_both, found_prefix): commands = [] command = None + def_vals = get_igmp_interface_defaults() for key, value in delta.items(): - if key == 'oif_source' or found_both or found_prefix: - pass - elif key == 'oif_prefix': - if delta.get('oif_source'): - command = CMDS.get('oif_prefix_source').format( - delta.get('oif_prefix'), delta.get('oif_source')) + if key == 'oif_ps': + for each in value: + if each in existing_oif_prefix_source: + existing_oif_prefix_source.remove(each) + else: + # add new prefix/sources + pf = each['prefix'] + src = '' + if 'source' in each.keys(): + src = each['source'] + if src: + commands.append(CMDS.get('oif_prefix_source').format(pf, src)) + else: + commands.append(CMDS.get('oif_prefix').format(pf)) + if existing_oif_prefix_source: + for each in existing_oif_prefix_source: + # remove stale prefix/sources + pf = each['prefix'] + src = '' + if 'source' in each.keys(): + src = each['source'] + if src: + commands.append('no ' + CMDS.get('oif_prefix_source').format(pf, src)) + else: + commands.append('no ' + CMDS.get('oif_prefix').format(pf)) + elif key == 'oif_routemap': + if value == 'default': + if existing.get(key): + command = 'no ' + CMDS.get(key).format(existing.get(key)) else: - command = CMDS.get('oif_prefix').format( - delta.get('oif_prefix')) + command = CMDS.get(key).format(value) elif value: - command = CMDS.get(key).format(value) + if value == 'default': + if def_vals.get(key) != existing.get(key): + command = CMDS.get(key).format(def_vals.get(key)) + else: + command = CMDS.get(key).format(value) elif not value: command = 'no {0}'.format(CMDS.get(key).format(value)) @@ -442,12 +459,12 @@ def get_igmp_interface_defaults(): return default -def config_default_igmp_interface(existing, delta, found_both, found_prefix): +def config_default_igmp_interface(existing, delta): commands = [] proposed = get_igmp_interface_defaults() delta = dict(set(proposed.items()).difference(existing.items())) if delta: - command = config_igmp_interface(delta, found_both, found_prefix) + command = config_igmp_interface(delta, existing, existing_oif_prefix_source=None) if command: for each in command: @@ -459,10 +476,9 @@ def config_default_igmp_interface(existing, delta, found_both, found_prefix): def config_remove_oif(existing, existing_oif_prefix_source): commands = [] command = None - if existing.get('routemap'): - command = 'no ip igmp static-oif route-map {0}'.format( - existing.get('routemap')) - if existing_oif_prefix_source: + if existing.get('oif_routemap'): + commands.append('no ip igmp static-oif route-map {0}'.format(existing.get('oif_routemap'))) + elif existing_oif_prefix_source: for each in existing_oif_prefix_source: if each.get('prefix') and each.get('source'): command = 'no ip igmp static-oif {0} source {1} '.format( @@ -495,16 +511,22 @@ def main(): report_llg=dict(type='bool'), immediate_leave=dict(type='bool'), oif_routemap=dict(required=False, type='str'), - oif_prefix=dict(required=False, type='str'), - oif_source=dict(required=False, type='str'), + oif_prefix=dict(required=False, type='str', removed_in_version='2.10'), + oif_source=dict(required=False, type='str', removed_in_version='2.10'), + oif_ps=dict(required=False, type='list', elements='dict'), restart=dict(type='bool', default=False), state=dict(choices=['present', 'absent', 'default'], default='present') ) argument_spec.update(nxos_argument_spec) + mutually_exclusive = [('oif_ps', 'oif_prefix'), + ('oif_ps', 'oif_source'), + ('oif_ps', 'oif_routemap'), + ('oif_prefix', 'oif_routemap')] module = AnsibleModule(argument_spec=argument_spec, + mutually_exclusive=mutually_exclusive, supports_check_mode=True) warnings = list() @@ -515,19 +537,19 @@ def main(): oif_prefix = module.params['oif_prefix'] oif_source = module.params['oif_source'] oif_routemap = module.params['oif_routemap'] + oif_ps = module.params['oif_ps'] - if oif_source: - if not oif_prefix: - module.fail_json(msg='oif_prefix required when setting oif_source') + if oif_source and not oif_prefix: + module.fail_json(msg='oif_prefix required when setting oif_source') + elif oif_source and oif_prefix: + oif_ps = [{'source': oif_source, 'prefix': oif_prefix}] + elif not oif_source and oif_prefix: + oif_ps = [{'prefix': oif_prefix}] intf_type = get_interface_type(interface) if get_interface_mode(interface, intf_type, module) == 'layer2': module.fail_json(msg='this module only works on Layer 3 interfaces') - if oif_prefix and oif_routemap: - module.fail_json(msg='cannot use oif_prefix AND oif_routemap.' - ' select one.') - existing = get_igmp_interface(module, interface) existing_copy = existing.copy() end_state = existing_copy @@ -543,7 +565,7 @@ def main(): module.fail_json(msg='Delete static-oif configurations on this ' 'interface if you want to use a routemap') - if oif_prefix and existing.get('oif_routemap'): + if oif_ps and existing.get('oif_routemap'): module.fail_json(msg='Delete static-oif route-map configuration ' 'on this interface if you want to config ' 'static entries') @@ -562,8 +584,6 @@ def main(): 'report_llg', 'immediate_leave', 'oif_routemap', - 'oif_prefix', - 'oif_source' ] changed = False @@ -581,41 +601,26 @@ def main(): for each in CANNOT_ABSENT: if each in proposed: module.fail_json(msg='only params: oif_prefix, oif_source, ' - 'oif_routemap can be used when ' + 'oif_ps, oif_routemap can be used when ' 'state=absent') - # delta check for all params except oif_prefix and oif_source + # delta check for all params except oif_ps delta = dict(set(proposed.items()).difference(existing.items())) - # now check to see there is a delta for prefix and source command option - found_both = False - found_prefix = False - - if existing_oif_prefix_source: - if oif_prefix and oif_source: - for each in existing_oif_prefix_source: - if (oif_prefix == each.get('prefix') and - oif_source == each.get('source')): - found_both = True - if not found_both: - delta['prefix'] = oif_prefix - delta['source'] = oif_source - elif oif_prefix: - for each in existing_oif_prefix_source: - if oif_prefix == each.get('prefix') and not each.get('source'): - found_prefix = True - if not found_prefix: - delta['prefix'] = oif_prefix + if oif_ps: + if oif_ps == ['default']: + delta['oif_ps'] = [] + else: + delta['oif_ps'] = oif_ps if state == 'present': if delta: - command = config_igmp_interface(delta, found_both, found_prefix) + command = config_igmp_interface(delta, existing, existing_oif_prefix_source) if command: commands.append(command) elif state == 'default': - command = config_default_igmp_interface(existing, delta, - found_both, found_prefix) + command = config_default_igmp_interface(existing, delta) if command: commands.append(command) elif state == 'absent': @@ -626,14 +631,10 @@ def main(): if command: commands.append(command) - command = config_default_igmp_interface(existing, delta, - found_both, found_prefix) + command = config_default_igmp_interface(existing, delta) if command: commands.append(command) - if module.params['restart']: - commands.append('restart igmp') - cmds = [] results = {} if commands: @@ -649,6 +650,10 @@ def main(): if 'configure' in cmds: cmds.pop(0) + if module.params['restart']: + cmd = {'command': 'restart igmp', 'output': 'text'} + run_commands(module, cmd) + results['proposed'] = proposed results['existing'] = existing_copy results['updates'] = cmds diff --git a/test/integration/targets/nxos_igmp_interface/tests/common/sanity.yaml b/test/integration/targets/nxos_igmp_interface/tests/common/sanity.yaml index 230bb3a1ebe..3c907286668 100644 --- a/test/integration/targets/nxos_igmp_interface/tests/common/sanity.yaml +++ b/test/integration/targets/nxos_igmp_interface/tests/common/sanity.yaml @@ -6,6 +6,9 @@ # Select interface for test - set_fact: intname="{{ nxos_int1 }}" +- set_fact: restart="true" + when: platform is not match("N35") + - name: "Enable feature PIM" nxos_feature: feature: pim @@ -13,6 +16,14 @@ provider: "{{ connection }}" ignore_errors: yes +- name: Put interface in default mode + nxos_config: + commands: + - "default interface {{ intname }}" + provider: "{{ connection }}" + match: none + ignore_errors: yes + - block: - name: put interface in L3 and enable PIM @@ -39,7 +50,10 @@ last_member_query_count: 4 report_llg: true immediate_leave: true - restart: false + group_timeout: 300 + # deprecated + oif_prefix: 239.255.255.2 + oif_source: 1.1.1.1 state: present provider: "{{ connection }}" register: result @@ -56,7 +70,78 @@ that: - "result.changed == false" - - name: Configure igmp interface with default value + - name: Configure igmp interface with some default values + nxos_igmp_interface: &sdef + interface: "{{ intname }}" + version: default + startup_query_interval: default + startup_query_count: default + robustness: default + querier_timeout: default + query_mrt: default + query_interval: default + last_member_qrt: default + last_member_query_count: default + group_timeout: default + oif_ps: + - {'prefix': '238.2.2.6'} + - {'prefix': '238.2.2.5'} + - {'source': '1.1.1.1', 'prefix': '238.2.2.5'} + state: present + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence - Configure igmp interface with some default values" + nxos_igmp_interface: *sdef + register: result + + - assert: *false + + - name: restart igmp + nxos_igmp_interface: &restart + interface: "{{ intname }}" + restart: "{{restart|default(omit)}}" + provider: "{{ connection }}" + + - name: Configure igmp interface with default oif_ps + nxos_igmp_interface: &defoif + interface: "{{ intname }}" + oif_ps: default + state: present + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence - Configure igmp interface with default oif_ps" + nxos_igmp_interface: *defoif + register: result + + - assert: *false + + - name: Configure igmp interface with oif_routemap + nxos_igmp_interface: &orm + interface: "{{ intname }}" + version: 3 + startup_query_interval: 60 + startup_query_count: 5 + robustness: 6 + oif_routemap: abcd + state: present + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence - Configure igmp interface with oif_routemap" + nxos_igmp_interface: *orm + register: result + + - assert: *false + + - name: Configure igmp interface with default state nxos_igmp_interface: &default interface: "{{ intname }}" state: default @@ -65,15 +150,30 @@ - assert: *true - - name: "Check Idempotence - Configure igmp interface with default value" + - name: "Check Idempotence - Configure igmp interface with default state" nxos_igmp_interface: *default register: result - assert: *false + - name: Configure igmp interface with absent state + nxos_igmp_interface: &absent + interface: "{{ intname }}" + state: absent + provider: "{{ connection }}" + register: result + + - assert: *true + + - name: "Check Idempotence - Configure igmp interface with absent state" + nxos_igmp_interface: *absent + register: result + + - assert: *false + always: - - name: Configure igmp interface with default value - nxos_igmp_interface: *default + - name: Configure igmp interface with absent state + nxos_igmp_interface: *absent register: result - name: Put interface in default mode