From 00e2c935d44857b7b033adf2373b2fd029ad3b11 Mon Sep 17 00:00:00 2001 From: mmudigon <62759545+mmudigon@users.noreply.github.com> Date: Mon, 8 Apr 2024 22:21:23 +0530 Subject: [PATCH] Fix for interface module, Issues 276 and 278 (#281) * Fix for interface module, Issues 276 and 278 * Fix for issues reported by ansible sanity test * Added code to handle SPEED for SVI and FEX interfaces --- plugins/modules/dcnm_interface.py | 120 +++++++++++++----- .../targets/prepare_dcnm_intf/tasks/main.yaml | 1 + .../fixtures/dcnm_intf_bunched_configs.json | 8 +- .../dcnm/fixtures/dcnm_intf_eth_configs.json | 20 +-- .../fixtures/dcnm_intf_mixed_configs.json | 2 +- .../fixtures/dcnm_intf_multi_configs.json | 4 +- .../dcnm_intf_multi_intf_configs.json | 6 +- tests/unit/modules/dcnm/test_dcnm_intf.py | 4 +- 8 files changed, 112 insertions(+), 53 deletions(-) diff --git a/plugins/modules/dcnm_interface.py b/plugins/modules/dcnm_interface.py index 88bdca107..7e675af2e 100644 --- a/plugins/modules/dcnm_interface.py +++ b/plugins/modules/dcnm_interface.py @@ -447,7 +447,6 @@ description: - Speed of the interface. type: str - choices: ['Auto', '100Mb', '1Gb', '10Gb', '25Gb', '40Gb', '100Gb'] default: Auto int_vrf: description: @@ -1852,6 +1851,21 @@ def dcnm_intf_dump_have_all(self): ) self.log_msg(f"HAVE ALL = {lhave_all}") + def dcnm_intf_xlate_speed(self, speed): + + # Controllers accept speed value in a particular format i.e. 1Gb, 100Gb etc. To make the playbook input + # case insensitive for speed, this routine translates the incoming speed to appropriate format. + + if speed == "": + return "" + + if speed.lower() == "auto": + return "auto".capitalize() + else: + comp = re.compile("([0-9]+)([a-zA-Z]+)") + match = comp.match(speed) + return str(match.group(1)) + match.group(2).capitalize() + # New Interfaces def dcnm_intf_get_if_name(self, name, if_type): @@ -2005,6 +2019,7 @@ def dcnm_intf_validate_port_channel_input(self, config): bpdu_guard=dict(type="str", default="true"), port_type_fast=dict(type="bool", default=True), mtu=dict(type="str", default="jumbo"), + speed=dict(type="str", default="Auto"), allowed_vlans=dict(type="str", default="none"), cmds=dict(type="list", elements="str"), description=dict(type="str", default=""), @@ -2018,6 +2033,7 @@ def dcnm_intf_validate_port_channel_input(self, config): bpdu_guard=dict(type="str", default="true"), port_type_fast=dict(type="bool", default=True), mtu=dict(type="str", default="jumbo"), + speed=dict(type="str", default="Auto"), access_vlan=dict(type="str", default=""), cmds=dict(type="list", elements="str"), description=dict(type="str", default=""), @@ -2033,6 +2049,7 @@ def dcnm_intf_validate_port_channel_input(self, config): ipv4_mask_len=dict(type="int", default=8), route_tag=dict(type="str", default=""), mtu=dict(type="int", default=9216, range_min=576, range_max=9216), + speed=dict(type="str", default="Auto"), cmds=dict(type="list", elements="str"), description=dict(type="str", default=""), admin_state=dict(type="bool", default=True), @@ -2077,6 +2094,7 @@ def dcnm_intf_validate_virtual_port_channel_input(self, cfg): bpdu_guard=dict(type="str", default="true"), port_type_fast=dict(type="bool", default=True), mtu=dict(type="str", default="jumbo"), + speed=dict(type="str", default="Auto"), peer1_allowed_vlans=dict(type="str", default="none"), peer2_allowed_vlans=dict(type="str", default="none"), peer1_cmds=dict(type="list"), @@ -2100,6 +2118,7 @@ def dcnm_intf_validate_virtual_port_channel_input(self, cfg): bpdu_guard=dict(type="str", default="true"), port_type_fast=dict(type="bool", default=True), mtu=dict(type="str", default="jumbo"), + speed=dict(type="str", default="Auto"), peer1_access_vlan=dict(type="str", default=""), peer2_access_vlan=dict(type="str", default=""), peer1_cmds=dict(type="list"), @@ -2141,6 +2160,7 @@ def dcnm_intf_validate_sub_interface_input(self, cfg): type="int", range_min=64, range_max=127, default=64 ), mtu=dict(type="int", range_min=576, range_max=9216, default=9216), + speed=dict(type="str", default="Auto"), cmds=dict(type="list", elements="str"), description=dict(type="str", default=""), admin_state=dict(type="bool", default=True), @@ -2165,6 +2185,7 @@ def dcnm_intf_validate_loopback_interface_input(self, cfg): int_vrf=dict(type="str", default="default"), ipv6_addr=dict(type="ipv6", default=""), route_tag=dict(type="str", default=""), + speed=dict(type="str", default="Auto"), cmds=dict(type="list", elements="str"), description=dict(type="str", default=""), admin_state=dict(type="bool", default=True), @@ -2572,6 +2593,11 @@ def dcnm_intf_get_pc_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["ADMIN_STATE"] = str( delem[profile]["admin_state"] ).lower() + intf["interfaces"][0]["nvPairs"][ + "SPEED" + ] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) def dcnm_intf_get_vpc_payload(self, delem, intf, profile): @@ -2710,6 +2736,9 @@ def dcnm_intf_get_vpc_payload(self, delem, intf, profile): delem[profile]["admin_state"] ).lower() intf["interfaces"][0]["nvPairs"]["INTF_NAME"] = ifname + intf["interfaces"][0]["nvPairs"]["SPEED"] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) def dcnm_intf_get_sub_intf_payload(self, delem, intf, profile): @@ -2754,6 +2783,9 @@ def dcnm_intf_get_sub_intf_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["ADMIN_STATE"] = str( delem[profile]["admin_state"] ).lower() + intf["interfaces"][0]["nvPairs"]["SPEED"] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) def dcnm_intf_get_loopback_payload(self, delem, intf, profile): @@ -2780,6 +2812,10 @@ def dcnm_intf_get_loopback_payload(self, delem, intf, profile): delem[profile]["admin_state"] ).lower() + intf["interfaces"][0]["nvPairs"]["SPEED"] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) + # Properties for mode 'lo' Loopback Interfaces if delem[profile]["mode"] == "lo": @@ -2813,8 +2849,12 @@ def dcnm_intf_get_loopback_payload(self, delem, intf, profile): # properties that can be modified. They will be updated from the # self.have dictionary to reflect the actual values later in the # code workflow that walks the want values and compares to have values. - intf["interfaces"][0]["nvPairs"]["DCI_ROUTING_PROTO"] = "PLACE_HOLDER" - intf["interfaces"][0]["nvPairs"]["DCI_ROUTING_TAG"] = "PLACE_HOLDER" + intf["interfaces"][0]["nvPairs"][ + "DCI_ROUTING_PROTO" + ] = "PLACE_HOLDER" + intf["interfaces"][0]["nvPairs"][ + "DCI_ROUTING_TAG" + ] = "PLACE_HOLDER" def dcnm_intf_get_eth_payload(self, delem, intf, profile): @@ -2835,9 +2875,6 @@ def dcnm_intf_get_eth_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["MTU"] = str( delem[profile]["mtu"] ) - intf["interfaces"][0]["nvPairs"]["SPEED"] = str( - delem[profile]["speed"] - ) intf["interfaces"][0]["nvPairs"]["ALLOWED_VLANS"] = delem[profile][ "allowed_vlans" ] @@ -2852,9 +2889,6 @@ def dcnm_intf_get_eth_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["MTU"] = str( delem[profile]["mtu"] ) - intf["interfaces"][0]["nvPairs"]["SPEED"] = str( - delem[profile]["speed"] - ) intf["interfaces"][0]["nvPairs"]["ACCESS_VLAN"] = delem[profile][ "access_vlan" ] @@ -2878,9 +2912,6 @@ def dcnm_intf_get_eth_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["MTU"] = str( delem[profile]["mtu"] ) - intf["interfaces"][0]["nvPairs"]["SPEED"] = str( - delem[profile]["speed"] - ) intf["interfaces"][0]["nvPairs"]["INTF_NAME"] = ifname if delem[profile]["mode"] == "monitor": intf["interfaces"][0]["nvPairs"]["INTF_NAME"] = ifname @@ -2903,9 +2934,6 @@ def dcnm_intf_get_eth_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["MTU"] = str( delem[profile]["mtu"] ) - intf["interfaces"][0]["nvPairs"]["SPEED"] = str( - delem[profile]["speed"] - ) intf["interfaces"][0]["nvPairs"]["INTF_NAME"] = ifname if delem[profile]["mode"] != "monitor": @@ -2921,6 +2949,11 @@ def dcnm_intf_get_eth_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["ADMIN_STATE"] = str( delem[profile]["admin_state"] ).lower() + intf["interfaces"][0]["nvPairs"][ + "SPEED" + ] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) def dcnm_intf_get_st_fex_payload(self, delem, intf, profile): @@ -2966,6 +2999,10 @@ def dcnm_intf_get_st_fex_payload(self, delem, intf, profile): delem[profile]["netflow_monitor"] ) + intf["interfaces"][0]["nvPairs"]["SPEED"] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) + def dcnm_intf_get_aa_fex_payload(self, delem, intf, profile): # Extract port id from the given name, which is of the form 'vPC300' @@ -3032,6 +3069,10 @@ def dcnm_intf_get_aa_fex_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["INTF_NAME"] = ifname + intf["interfaces"][0]["nvPairs"]["SPEED"] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) + def dcnm_intf_get_svi_payload(self, delem, intf, profile): # Extract port id from the given name, which is of the form 'vlan300' @@ -3135,8 +3176,11 @@ def dcnm_intf_get_svi_payload(self, delem, intf, profile): intf["interfaces"][0]["nvPairs"]["INTF_NAME"] = ifname - # we don't need SPEED for SVI interfaces - intf["interfaces"][0]["nvPairs"].pop("SPEED") + intf["interfaces"][0]["nvPairs"][ + "SPEED" + ] = self.dcnm_intf_xlate_speed( + str(delem[profile].get("speed", "")) + ) # New Interfaces def dcnm_get_intf_payload(self, delem, sw): @@ -3432,7 +3476,7 @@ def dcnm_intf_compare_elements( if (state == "replaced") or (state == "overridden"): # Special handling is required for mode 'mpls' loopback interfaces. # They will contain either of the following two read_only properties. - if k in ['DCI_ROUTING_PROTO', 'DCI_ROUTING_TAG']: + if k in ["DCI_ROUTING_PROTO", "DCI_ROUTING_TAG"]: return "copy_and_add" return "add" @@ -3560,7 +3604,12 @@ def dcnm_intf_compare_want_and_have(self, state): if ik == "nvPairs": nv_keys = list(want[k][0][ik].keys()) if "SPEED" in nv_keys: - nv_keys.remove("SPEED") + # Remove 'SPEED' only if it is not included in 'have'. + if ( + d[k][index][ik].get("SPEED", None) + is None + ): + nv_keys.remove("SPEED") for nk in nv_keys: # HAVE may have an entry with a list # of interfaces. Check all the # interface entries for a match. Even if one entry matches do not @@ -4061,12 +4110,15 @@ def dcnm_intf_get_diff_overridden(self, cfg): del_list.append(have) if str(deploy).lower() == "true": - self.diff_delete_deploy[ - self.int_index[have["ifType"]] - ].append(delem) - self.changed_dict[0]["delete_deploy"].append( - copy.deepcopy(delem) - ) + if (have["complianceStatus"] == "In-Sync") or ( + have["complianceStatus"] == "Pending" + ): + self.diff_delete_deploy[ + self.int_index[have["ifType"]] + ].append(delem) + self.changed_dict[0]["delete_deploy"].append( + copy.deepcopy(delem) + ) for intf in defer_list: # Check if the 'source' for the ethernet interface is one of the interfaces that is already deleted. @@ -4224,12 +4276,18 @@ def dcnm_intf_get_diff_deleted(self): self.changed_dict[0][ "replaced" ].append(copy.deepcopy(uelem)) - delem["serialNumber"] = intf[ - "serialNumber" - ] - delem["ifName"] = if_name - delem["fabricName"] = self.fabric - self.diff_deploy.append(delem) + if ( + str( + cfg.get("deploy", "true") + ).lower() + == "true" + ): + delem["serialNumber"] = intf[ + "serialNumber" + ] + delem["ifName"] = if_name + delem["fabricName"] = self.fabric + self.diff_deploy.append(delem) else: intf_payload = self.dcnm_intf_get_intf_info_from_dcnm( intf diff --git a/tests/integration/targets/prepare_dcnm_intf/tasks/main.yaml b/tests/integration/targets/prepare_dcnm_intf/tasks/main.yaml index 1d817b4eb..20f2ab51f 100644 --- a/tests/integration/targets/prepare_dcnm_intf/tasks/main.yaml +++ b/tests/integration/targets/prepare_dcnm_intf/tasks/main.yaml @@ -175,6 +175,7 @@ "peerOneId": "{{ ansible_cxt_fabric_sno1 }}", "peerTwoId": "{{ ansible_cxt_fabric_sno2 }}", "useVirtualPeerlink": false, + "templateName": "vpc_pair", "nvPairs": { "DOMAIN_ID": "{{ ansible_cxt_vpc_domain_id }}", "PEER1_KEEP_ALIVE_LOCAL_IP": "{{ ansible_cxt_fabric_sw1 }}", diff --git a/tests/unit/modules/dcnm/fixtures/dcnm_intf_bunched_configs.json b/tests/unit/modules/dcnm/fixtures/dcnm_intf_bunched_configs.json index d54461084..fc901012e 100644 --- a/tests/unit/modules/dcnm/fixtures/dcnm_intf_bunched_configs.json +++ b/tests/unit/modules/dcnm/fixtures/dcnm_intf_bunched_configs.json @@ -210,7 +210,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "description": "eth interface acting as routed" }, "name": "eth1/32", @@ -236,7 +236,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "description": "eth interface acting as routed" }, "name": "eth1/22", @@ -261,7 +261,7 @@ "admin_state": "False", "ifname": "Ethernet1/13", "route_tag": "", - "speed": "auto", + "speed": "Auto", "cmds": [ "no shutdown" ], @@ -289,7 +289,7 @@ "admin_state": "False", "ifname": "Ethernet1/14", "route_tag": "", - "speed": "auto", + "speed": "Auto", "cmds": [ "no shutdown" ], diff --git a/tests/unit/modules/dcnm/fixtures/dcnm_intf_eth_configs.json b/tests/unit/modules/dcnm/fixtures/dcnm_intf_eth_configs.json index df177a308..c75b3660c 100644 --- a/tests/unit/modules/dcnm/fixtures/dcnm_intf_eth_configs.json +++ b/tests/unit/modules/dcnm/fixtures/dcnm_intf_eth_configs.json @@ -95,7 +95,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/30", "fabric": "test_fabric" }, @@ -121,7 +121,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "fabric": "test_fabric" }, "name": "eth1/31", @@ -147,7 +147,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "description": "eth interface acting as routed" }, "name": "eth1/32", @@ -172,7 +172,7 @@ "admin_state": "False", "ifname": "Ethernet1/33", "route_tag": "", - "speed": "auto", + "speed": "Auto", "cmds": [ "no shutdown" ], @@ -341,7 +341,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/30", "fabric": "test_fabric" }, @@ -367,7 +367,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "fabric": "test_fabric" }, "name": "eth1/31", @@ -393,7 +393,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "description": "eth interface acting as routed" }, "name": "eth1/32", @@ -418,7 +418,7 @@ "admin_state": "False", "ifname": "Ethernet1/33", "route_tag": "", - "speed": "auto", + "speed": "Auto", "cmds": [ "no shutdown" ], @@ -463,7 +463,7 @@ "no shutdown", "no shutdown" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/30", "fabric": "test_fabric" }, @@ -491,7 +491,7 @@ "no shutdown", "no shutdown" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/2", "fabric": "test_fabric" }, diff --git a/tests/unit/modules/dcnm/fixtures/dcnm_intf_mixed_configs.json b/tests/unit/modules/dcnm/fixtures/dcnm_intf_mixed_configs.json index e391c78a9..c29fbc7d3 100644 --- a/tests/unit/modules/dcnm/fixtures/dcnm_intf_mixed_configs.json +++ b/tests/unit/modules/dcnm/fixtures/dcnm_intf_mixed_configs.json @@ -123,7 +123,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/30", "fabric": "test_fabric" }, diff --git a/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_configs.json b/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_configs.json index eb1501551..d087996ba 100644 --- a/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_configs.json +++ b/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_configs.json @@ -162,7 +162,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/10", "fabric": "test_fabric" }, @@ -345,7 +345,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "description": "eth interface acting as routed" }, "name": "eth1/32", diff --git a/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_intf_configs.json b/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_intf_configs.json index c80b7d293..e906933c0 100644 --- a/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_intf_configs.json +++ b/tests/unit/modules/dcnm/fixtures/dcnm_intf_multi_intf_configs.json @@ -162,7 +162,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/10", "fabric": "test_fabric" }, @@ -307,7 +307,7 @@ "cmds": [ "spanning-tree bpduguard enable" ], - "speed": "auto", + "speed": "Auto", "ifname": "Ethernet1/10", "fabric": "test_fabric" }, @@ -488,7 +488,7 @@ "cmds": [ "no shutdown" ], - "speed": "auto", + "speed": "Auto", "description": "eth interface acting as routed" }, "name": "eth1/32", diff --git a/tests/unit/modules/dcnm/test_dcnm_intf.py b/tests/unit/modules/dcnm/test_dcnm_intf.py index 6ceb0bb17..8901d7be9 100644 --- a/tests/unit/modules/dcnm/test_dcnm_intf.py +++ b/tests/unit/modules/dcnm/test_dcnm_intf.py @@ -3170,7 +3170,7 @@ def test_dcnm_intf_subint_overridden_existing(self): ] rep_if_names = ["ethernet1/1", "ethernet1/2", "ethernet3/2"] - ovr_if_names = ["Ethernet1/25.1"] + ovr_if_names = ["ethernet1/25.1"] for intf in result["diff"][0]["deleted"]: self.assertEqual((intf["ifName"].lower() in del_if_names), True) @@ -3758,7 +3758,7 @@ def test_dcnm_intf_vpc_overridden_existing(self): "ethernet1/2", "ethernet3/2", ] - ovr_if_names = ["vPC750"] + ovr_if_names = ["vpc750"] for intf in result["diff"][0]["deleted"]: self.assertEqual((intf["ifName"].lower() in del_if_names), True)