Replaced changed flag #424 (#448)

* Handled edge cases for network attachment states, corrected replaced changed flag for empty attachments

* fixed deletion of pending networks with attachments. Updated templates to ensure deploy flag is correct. Removed test case which is no longer applicable

* modified unit test

* set a consistent timer for wait_for_del_ready

Author: neiljohn-gh

plugins/modules/dcnm_network.py

@@ -495,6 +495,8 @@ class DcnmNetwork:
             "GET_NET": "/rest/top-down/fabrics/{}/networks",
             "GET_NET_NAME": "/rest/top-down/fabrics/{}/networks/{}",
             "GET_VLAN": "/rest/resource-manager/vlan/{}?vlanUsageType=TOP_DOWN_NETWORK_VLAN",
+            "GET_NET_STATUS": "/rest/top-down/fabrics/{}/networks/{}/status",
+            "GET_NET_SWITCH_DEPLOY": "/rest/top-down/fabrics/networks/deploy"
         },
         12: {
             "GET_VRF": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/top-down/fabrics/{}/vrfs",
@@ -504,6 +506,8 @@ class DcnmNetwork:
             "GET_NET": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/top-down/fabrics/{}/networks",
             "GET_NET_NAME": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/top-down/fabrics/{}/networks/{}",
             "GET_VLAN": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/resource-manager/vlan/{}?vlanUsageType=TOP_DOWN_NETWORK_VLAN",
+            "GET_NET_STATUS": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/top-down/fabrics/{}/networks/{}/status",
+            "GET_NET_SWITCH_DEPLOY": "/appcenter/cisco/ndfc/api/v1/lan-fabric/rest/top-down/networks/deploy"
         },
     }
 
@@ -593,6 +597,8 @@ def find_dict_in_list_by_key_value(search: list, key: str, value: str):
         # -> None
         ```
         """
+        if search is None:
+            return None
         match = (d for d in search if d[key] == value)
         return next(match, None)
 
@@ -1466,10 +1472,10 @@ def get_have(self):
             dep_net = ""
             for attach in attach_list:
                 torlist = []
-                attach_state = False if attach["lanAttachState"] == "NA" else True
-                deploy = attach["isLanAttached"]
+                attach_state = bool(attach.get("isLanAttached", False))
+                deploy = attach_state
                 deployed = False
-                if bool(deploy) and (attach["lanAttachState"] == "OUT-OF-SYNC" or attach["lanAttachState"] == "PENDING"):
+                if attach_state and (attach["lanAttachState"] == "OUT-OF-SYNC" or attach["lanAttachState"] == "PENDING"):
                     deployed = False
                 else:
                     deployed = True
@@ -1799,16 +1805,25 @@ def get_diff_replace(self):
                     diff_attach.append(r_net_dict)
                     all_nets += have_a["networkName"] + ","
 
+        if all_nets:
+            modified_all_nets = copy.deepcopy(all_nets[:-1].split(","))
+            # If the playbook sets the deploy key to False, then we need to remove the network from the deploy list.
+            for net in all_nets[:-1].split(","):
+                want_net_data = self.find_dict_in_list_by_key_value(search=self.config, key="net_name", value=net)
+                if (want_net_data is not None) and (want_net_data.get("deploy") is False):
+                    modified_all_nets.remove(net)
+            all_nets = ",".join(modified_all_nets)
+
         if not all_nets:
             self.diff_create = diff_create
             self.diff_attach = diff_attach
             self.diff_deploy = diff_deploy
             return warn_msg
 
         if not self.diff_deploy:
-            diff_deploy.update({"networkNames": all_nets[:-1]})
+            diff_deploy.update({"networkNames": all_nets})
         else:
-            nets = self.diff_deploy["networkNames"] + "," + all_nets[:-1]
+            nets = self.diff_deploy["networkNames"] + "," + all_nets
             diff_deploy.update({"networkNames": nets})
 
         self.diff_create = diff_create
@@ -2311,28 +2326,74 @@ def get_diff_query(self):
 
         self.query = query
 
+    def detach_and_deploy_for_del(self, net):
+        method = "GET"
+
+        payload_net = {}
+        deploy_payload = {}
+        payload_net["networkName"] = net["networkName"]
+        payload_net["lanAttachList"] = []
+        attach_list = net["switchList"]
+        for atch in attach_list:
+            payload_atch = {}
+            if atch["lanAttachedState"].upper() == "PENDING":
+                payload_atch["serialNumber"] = atch["serialNumber"]
+                payload_atch["networkName"] = net["networkName"]
+                payload_atch["fabric"] = net["fabric"]
+                payload_atch["deployment"] = False
+                payload_net["lanAttachList"].append(payload_atch)
+
+                deploy_payload[atch["serialNumber"]] = net["networkName"]
+
+        if payload_net["lanAttachList"]:
+            payload = [payload_net]
+            method = "POST"
+            path = self.paths["GET_NET"].format(self.fabric) + "/attachments"
+            resp = dcnm_send(self.module, method, path, json.dumps(payload))
+            self.result["response"].append(resp)
+            fail, dummy_changed = self.handle_response(resp, "attach")
+            if fail:
+                self.failure(resp)
+
+        method = "POST"
+        path = self.paths["GET_NET_SWITCH_DEPLOY"].format(self.fabric)
+        resp = dcnm_send(self.module, method, path, json.dumps(deploy_payload))
+        self.result["response"].append(resp)
+        fail, dummy_changed = self.handle_response(resp, "deploy")
+        if fail:
+            self.failure(resp)
+
     def wait_for_del_ready(self):
 
         method = "GET"
         if self.diff_delete:
             for net in self.diff_delete:
                 state = False
-                path = self.paths["GET_NET_ATTACH"].format(self.fabric, net)
-                while not state:
+                path = self.paths["GET_NET_STATUS"].format(self.fabric, net)
+                retry = max(100 // self.WAIT_TIME_FOR_DELETE_LOOP, 1)
+                deploy_started = False
+                while not state and retry >= 0:
+                    retry -= 1
                     resp = dcnm_send(self.module, method, path)
                     state = True
                     if resp["DATA"]:
-                        attach_list = resp["DATA"][0]["lanAttachList"]
+                        if resp["DATA"]["networkStatus"].upper() == "PENDING" and not deploy_started:
+                            self.detach_and_deploy_for_del(resp["DATA"])
+                            deploy_started = True
+                        attach_list = resp["DATA"]["switchList"]
                         for atch in attach_list:
-                            if atch["lanAttachState"] == "OUT-OF-SYNC" or atch["lanAttachState"] == "FAILED":
+                            if atch["lanAttachedState"].upper() == "OUT-OF-SYNC" or atch["lanAttachedState"].upper() == "FAILED":
                                 self.diff_delete.update({net: "OUT-OF-SYNC"})
                                 break
-                            if atch["lanAttachState"] != "NA":
+                            if atch["lanAttachedState"].upper() != "NA":
                                 self.diff_delete.update({net: "DEPLOYED"})
                                 state = False
                                 time.sleep(self.WAIT_TIME_FOR_DELETE_LOOP)
                                 break
                             self.diff_delete.update({net: "NA"})
+                if retry < 0:
+                    self.diff_delete.update({net: "TIMEOUT"})
+                    return False
 
             return True
 
@@ -2376,7 +2437,8 @@ def push_to_remote(self, is_rollback=False):
 
             for d_a in self.diff_detach:
                 for v_a in d_a["lanAttachList"]:
-                    del v_a["is_deploy"]
+                    if v_a.get("is_deploy"):
+                        del v_a["is_deploy"]
 
             resp = dcnm_send(self.module, method, detach_path, json.dumps(self.diff_detach))
             self.result["response"].append(resp)
@@ -2396,7 +2458,7 @@ def push_to_remote(self, is_rollback=False):
             # the state of the network is "OUT-OF-SYNC"
             self.wait_for_del_ready()
             for net, state in self.diff_delete.items():
-                if state == "OUT-OF-SYNC":
+                if state.upper() == "OUT-OF-SYNC":
                     resp = dcnm_send(self.module, method, deploy_path, json.dumps(self.diff_undeploy))
 
             self.result["response"].append(resp)
@@ -2410,9 +2472,14 @@ def push_to_remote(self, is_rollback=False):
         method = "DELETE"
         del_failure = ""
         if self.diff_delete and self.wait_for_del_ready():
+            resp = ""
             for net, state in self.diff_delete.items():
-                if state == "OUT-OF-SYNC":
+                if state.upper() == "OUT-OF-SYNC" or state == "TIMEOUT":
                     del_failure += net + ","
+                    if state == "TIMEOUT":
+                        resp = "Timeout waiting for network to be in delete ready state.\n"
+                    if state == "OUT-OF-SYNC":
+                        resp += "Network is out of sync.\n"
                     continue
                 delete_path = path + "/" + net
                 resp = dcnm_send(self.module, method, delete_path)
@@ -2498,7 +2565,8 @@ def push_to_remote(self, is_rollback=False):
 
             for d_a in self.diff_attach:
                 for v_a in d_a["lanAttachList"]:
-                    del v_a["is_deploy"]
+                    if v_a.get("is_deploy"):
+                        del v_a["is_deploy"]
 
             for attempt in range(0, 50):
                 resp = dcnm_send(self.module, method, attach_path, json.dumps(self.diff_attach))

tests/integration/targets/dcnm_network/templates/self-contained-tests/net1_dhcp_conf.j2

@@ -26,4 +26,4 @@
   attach:
   - ip_address: "{{ test_data_common.sw1 }}"
     ports: []
-  deploy: false
+  deploy: {{ test_data_common.deploy | bool }}

tests/integration/targets/dcnm_network/tests/dcnm/replaced.yaml

@@ -127,12 +127,11 @@
   register: result
   tags: replaced
 
-# Possible bug, changed flag is not set to true when deleting attachments
-# - name: REPLACED - TC1 - ASSERT - Check if changed flag is true
-#   assert:
-#     that:
-#     - result.changed == true
-#   tags: replaced
+- name: REPLACED - TC1 - ASSERT - Check if changed flag is true
+  assert:
+    that:
+    - result.changed == true
+  tags: replaced
 
 - name: REPLACED - TC1 - QUERY - Get network state in NDFC
   cisco.dcnm.dcnm_network:
@@ -254,11 +253,11 @@
   register: result
   tags: replaced
 
-# - name: REPLACED - TC3 - ASSERT - Check if changed flag is true
-#   assert:
-#     that:
-#     - result.changed == true
-#   tags: replaced
+- name: REPLACED - TC3 - ASSERT - Check if changed flag is true
+  assert:
+    that:
+    - result.changed == true
+  tags: replaced
 
 - name: REPLACED - TC3 - QUERY - Get network state in NDFC
   cisco.dcnm.dcnm_network:

tests/integration/targets/dcnm_network/tests/dcnm/self-contained-tests/ingress_replication_networks.yaml

@@ -129,6 +129,6 @@
 
   - name: MERGED - setup - remove any networks
     cisco.dcnm.dcnm_network:
-      fabric: "{{ test_fabric }}"
+      fabric: "{{ test_data_common.fabric }}"
       state: deleted
   when: test_ing_fabric is defined

tests/integration/targets/dcnm_network/tests/dcnm/self-contained-tests/sm_dhcp_update.yaml

@@ -50,7 +50,6 @@
     dcnm_network_merged_net1_dhcp_conf: "{{ lookup('file', '{{ test_data.net1_dhcp_conf_file }}') | from_yaml }}"
   delegate_to: localhost
 
-# below deploy is true, reason is in comment above idempotence checl
 - name: SM_DHCP_UPDATE - Create New Network with many params
   cisco.dcnm.dcnm_network: &conf
     fabric: "{{ test_data_common.fabric }}"
@@ -83,7 +82,7 @@
 
 - name: SM_DHCP_UPDATE - Change dhcp parameter values
   cisco.dcnm.dcnm_network:
-    fabric: "{{ test_fabric }}"
+    fabric: "{{ test_data_common.fabric }}"
     state: merged
     config: "{{ dcnm_network_merged_net1_dhcp_changed_conf }}"
   register: result

tests/integration/targets/dcnm_network/tests/dcnm/self-contained-tests/sm_mcast_update.yaml

@@ -83,7 +83,7 @@
 
 - name: SM_MCAST_UPDATE - Change mcast parameter values
   cisco.dcnm.dcnm_network:
-    fabric: "{{ test_fabric }}"
+    fabric: "{{ test_data_common.fabric }}"
     state: merged
     config: "{{ dcnm_network_merged_net1_mcast_changed_conf }}"
   register: result

tests/integration/targets/dcnm_network/tests/dcnm/self-contained-tests/so_dhcp_update.yaml

@@ -50,7 +50,6 @@
     dcnm_network_merged_double_net_dhcp_conf: "{{ lookup('file', '{{ test_data.double_net_dhcp_conf_file }}') | from_yaml }}"
   delegate_to: localhost
 
-# below deploy is true, reason is in comment above idempotence checl
 - name: SO_DHCP_UPDATE - Create New Network with many params
   cisco.dcnm.dcnm_network: &conf
     fabric: "{{ test_data_common.fabric }}"
@@ -83,7 +82,7 @@
 
 - name: SO_DHCP_UPDATE - Override network config
   cisco.dcnm.dcnm_network:
-    fabric: "{{ test_fabric }}"
+    fabric: "{{ test_data_common.fabric }}"
     state: overridden
     config: "{{ dcnm_network_merged_double_net_dhcp_changed_conf }}"
   register: result

tests/integration/targets/dcnm_network/tests/dcnm/self-contained-tests/so_mcast_update.yaml

@@ -83,7 +83,7 @@
 
 - name: SO_MCAST_UPDATE - Override network config
   cisco.dcnm.dcnm_network:
-    fabric: "{{ test_fabric }}"
+    fabric: "{{ test_data_common.fabric }}"
     state: overridden
     config: "{{ dcnm_network_merged_double_net_mcast_changed_conf }}"
   register: result