Fix(cv_device_v3): reconciled configlets are not treated specially (#667

)

* Feat(cv_device_v3): add configuration validation support

* Bug(cv_device_v3): reconciled configlets are not treated specially

* revert commit from older stale PR

* revert old device validation commit

* adding molecule test

* add fixtures and read vars from it + cleanup

* updating condition

* Remove reconciled configlet with strict mode

Co-authored-by: alexeygorbunov <[email protected]>

* Experiment with different method

* rewriting the logic

Co-authored-by: Claus Holbech <[email protected]>

* removing commented experiments

* Attempting to fix pylint error

---------

Co-authored-by: alexeygorbunov <[email protected]>
Co-authored-by: Claus Holbech <[email protected]>
Co-authored-by: Sugetha Chandhrasekar <[email protected]>

ansible_collections/arista/cvp/molecule/cv_device_v3/converge.yml

@@ -5,6 +5,9 @@
 - name: Test cv_device_v3 module
   import_playbook: test_apply_detach_configlet.yml
 
+- name: Test cv_device_v3 module
+  import_playbook: test_reconciled_configlet.yml
+
 - name: Test cv_device_v3 module
   import_playbook: test_apply_detach_bundle.yml
 

ansible_collections/arista/cvp/molecule/cv_device_v3/reconcile.py

@@ -0,0 +1,39 @@
+#!/usr/bin/env python
+# Copyright (c) 2023 Arista Networks, Inc.
+# Use of this source code is governed by the Apache License 2.0
+# that can be found in the LICENSE file.
+from cvprac.cvp_client import CvpClient
+import yaml
+import requests.packages.urllib3
+requests.packages.urllib3.disable_warnings()
+
+# Setting variables
+RECONCILE = 'RECONCILE_' # Prefix for the configlet name
+fixture_file = 'molecule/fixtures/cv_device_v3.yaml'
+
+with open(fixture_file, encoding="utf-8") as f:
+    dut = yaml.safe_load(f)
+
+# load password from fixtures otherwise assume it's and ATD environemnt and read the password from
+# the config file
+if len(dut[0]["password"]) > 0:
+    password = dut[0]["password"]
+else:
+    config_file = "/home/coder/.config/code-server/config.yaml"
+    with open(config_file, encoding="utf-8") as f:
+        password = yaml.safe_load(f)["password"]
+
+# Connect to CloudVision
+clnt = CvpClient()
+clnt.set_log_level(log_level='WARNING')
+clnt.connect([dut[0]["node"]], dut[0]["username"], password)
+
+# Store device information
+device = clnt.api.get_device_by_serial(dut[0]["device"])
+dev_mac = device["systemMacAddress"]
+
+# Reconcile device configuration
+rc = clnt.api.get_device_configuration(dev_mac)
+name = RECONCILE + device['serialNumber']
+update = clnt.api.update_reconcile_configlet(dev_mac, rc, "", name, True)
+addcfg = clnt.api.apply_configlets_to_device("auto-reconciling", device,[update['data']])

ansible_collections/arista/cvp/molecule/cv_device_v3/test_reconciled_configlet.yml

@@ -0,0 +1,126 @@
+---
+- name: Test cv_device_v3
+  hosts: CloudVision
+  connection: local
+  gather_facts: no
+  vars:
+    device_name: s1-leaf1
+
+    cvp_configlets:
+      configlet1: '! This is first configlet'
+      configlet2: '! This is second configlet'
+      configlet3: 'alias shover show version'
+
+    cvp_devices_apply_configlet:
+      - fqdn: "{{device_name}}"
+        parentContainerName: "{{cv_facts_v3_result.data.cvp_devices[0].parentContainerName}}"
+        configlets:
+          - 'configlet1'
+          - 'configlet2'
+          - 'configlet3'
+
+    cvp_devices_detach_configlet:
+      - fqdn: "{{device_name}}"
+        parentContainerName: "{{cv_facts_v3_result.data.cvp_devices[0].parentContainerName}}"
+        configlets: "{{cv_facts_v3_result.data.cvp_devices[0].configlets + ['configlet1', 'configlet2']}}"
+
+    cvp_devices_deploy:
+      - fqdn: "{{device_name}}"    # device must be in undefined container
+        parentContainerName: "{{cv_facts_v3_result.data.cvp_devices[0].parentContainerName}}"
+        configlets: "{{cv_facts_v3_result.data.cvp_devices[0].configlets}}"
+
+    test_reconcile_misorder:
+      - fqdn: "{{device_name}}"
+        parentContainerName: "{{cv_facts_v3_result.data.cvp_devices[0].parentContainerName}}"
+        configlets: "{{cv_facts_v3_result.data.cvp_devices[0].configlets + ['configlet1', 'RECONCILE_' + device_name, 'configlet2']}}"
+
+  tasks:
+    - name: Collect devices facts from {{ inventory_hostname }}
+      arista.cvp.cv_facts_v3:
+        facts:
+          - devices
+        regexp_filter: "{{ device_name }}"
+      register: cv_facts_v3_result
+
+    - name: "Push config"
+      arista.cvp.cv_configlet_v3:
+        configlets: "{{ cvp_configlets }}"
+        state: present
+
+    - name: Apply configlet on {{ inventory_hostname }}
+      arista.cvp.cv_device_v3:
+        devices: '{{ cvp_devices_apply_configlet }}'
+        state: present
+      register: CV_DEVICE_V3_RESULT
+
+    - name: "Check apply_configlet with apply_mode loose"
+      ansible.builtin.assert:
+        that:
+          - CV_DEVICE_V3_RESULT.changed == true
+          - CV_DEVICE_V3_RESULT.configlets_attached.changed == true
+          - CV_DEVICE_V3_RESULT.configlets_attached.configlets_attached_count == 3
+          - CV_DEVICE_V3_RESULT.configlets_attached.configlets_attached_list == ["s1-leaf1_configlet_attached"]
+          - CV_DEVICE_V3_RESULT.configlets_attached.success == true
+          - CV_DEVICE_V3_RESULT.configlets_attached.taskIds != []
+
+    - name: "Detach configlet from {{ inventory_hostname }}"
+      arista.cvp.cv_device_v3:
+        devices: '{{ cvp_devices_detach_configlet }}'
+        state: present
+        apply_mode: strict
+      register: CV_DEVICE_V3_RESULT
+
+    - name: Run Python script to reconcile the device
+      ansible.builtin.command: python3 molecule/cv_device_v3/reconcile.py
+
+    - name: Execute Task for detach configlet
+      arista.cvp.cv_task_v3:
+        tasks:
+          - "{{ CV_DEVICE_V3_RESULT.taskIds[0] }}"
+        state: cancelled
+
+    # Regardless of where the user specifies the reconciled configlets position, cv_device_v3
+    # will make sure to move to the tail end of the configlet list, which should not trigger
+    # an empty task.
+    - name: Testing misordering the reconciled configlet
+      arista.cvp.cv_device_v3:
+        devices: '{{ test_reconcile_misorder }}'
+        state: present
+      register: reconciled_misorder_result
+
+    - name: Checking that no task was generated
+      ansible.builtin.assert:
+        that:
+          - reconciled_misorder_result.changed == false
+          - reconciled_misorder_result.configlets_attached.changed == false
+          - reconciled_misorder_result.configlets_attached.configlets_attached_count == 0
+          - reconciled_misorder_result.success == false
+          - reconciled_misorder_result.configlets_attached.taskIds == []
+
+    - name: "Resetting original state on device: {{ inventory_hostname }}"
+      arista.cvp.cv_device_v3:
+        devices: '{{ cvp_devices_deploy }}'
+        apply_mode: strict
+        state: present
+      register: CV_DEVICE_V3_RESULT
+
+    - name: "Checking if the reset was successful"
+      ansible.builtin.assert:
+        that:
+          - CV_DEVICE_V3_RESULT.changed == true
+          - CV_DEVICE_V3_RESULT.configlets_attached.changed == true
+          - CV_DEVICE_V3_RESULT.configlets_attached.configlets_attached_count == 2
+          - CV_DEVICE_V3_RESULT.configlets_attached.configlets_attached_list == ["s1-leaf1_configlet_attached"]
+          - CV_DEVICE_V3_RESULT.configlets_attached.success == true
+          - CV_DEVICE_V3_RESULT.configlets_attached.taskIds != []
+
+    - name: "Delete config"
+      arista.cvp.cv_configlet_v3:
+        configlets: "{{ cvp_configlets | combine({'RECONCILE_' + device_name: ''}) }}"
+        state: absent
+
+    - name: Execute the task to reset the device back to its original state
+      arista.cvp.cv_task_v3:
+        tasks:
+          - "{{ CV_DEVICE_V3_RESULT.configlets_attached.taskIds[0] }}"
+      when: CV_DEVICE_V3_RESULT.success is true

ansible_collections/arista/cvp/molecule/fixtures/cv_device_v3.yaml

@@ -0,0 +1,4 @@
+- node : 192.168.0.5
+  username: arista
+  password: ""
+  device: s1-leaf1

ansible_collections/arista/cvp/plugins/module_utils/device_tools.py

@@ -526,7 +526,7 @@ def __get_reordered_configlets_list(
         self, configlet_applied_to_device_list, configlet_playbook_list
     ):
         """
-        __get_reordered_configlets_list Provides mechanism to reoder the configlet lists.
+        __get_reordered_configlets_list Provides mechanism to reorder the configlet lists.
 
         Extract information from CV configlet mapper and save as a cache in instance.
 
@@ -552,11 +552,11 @@ def __get_reordered_configlets_list(
                 MODULE_LOGGER.error(error_message)
                 self.__ansible.fail_json(msg=error_message)
 
-            # If the configlet is not applied, add it to the new list
-            if configlet not in [x.name for x in configlet_applied_to_device_list]:
+            # If the configlet is not applied, add it to the new list avoiding duplicates.
+            if configlet not in [x.name for x in configlet_applied_to_device_list] and new_configlet not in new_configlets_list:
                 new_configlets_list.append(new_configlet)
 
-            # If the confilet is already applied, remove it from the main list and add it to the end of the new list
+            # If the configlet is already applied, remove it from the main list and add it to the end of the new list
             else:
                 MODULE_LOGGER.debug(
                     "Removing the configlet %s from the current configlet list and adding it to the new list.",
@@ -565,13 +565,33 @@ def __get_reordered_configlets_list(
                 for x in configlet_applied_to_device_list:
                     if x.name == configlet:
                         configlet_applied_to_device_list.remove(x)
-                        new_configlets_list.append(new_configlet)
+                        if new_configlet not in new_configlets_list:
+                            new_configlets_list.append(new_configlet)
+                            break
         configlets_attached_get_configlet_info = [
             self.__get_configlet_info(configlet_name=x.name)
             for x in configlet_applied_to_device_list
         ]
+
         # Joining the 2 new list (configlets already present + new configlet in right order)
-        return configlets_attached_get_configlet_info + new_configlets_list
+        reordered_configlets_list = configlets_attached_get_configlet_info + new_configlets_list
+        MODULE_LOGGER.debug("reordered_configlets_list %s", str(reordered_configlets_list))
+        # Find any reconcile configlet and move to the end of the reordered list.
+        reconciled_configlet_indexes = [index for index, configlet in enumerate(reordered_configlets_list) if configlet["reconciled"]]
+        reconciled_configlet_indexes.reverse()
+        reconciled_configlets = [reordered_configlets_list.pop(index) for index in reconciled_configlet_indexes]
+        reordered_configlets_list.extend(reconciled_configlets)
+
+        if len(reconciled_configlet_indexes) > 1:
+            reconcile_configlet_names = ", ".join([configlet.name for configlet in reconciled_configlets])
+            error_message = (
+                "Two 'reconcile' configlets '%s' are assigned to one device, which is not supported by CloudVision.",
+                reconcile_configlet_names,
+            )
+            MODULE_LOGGER.error(error_message)
+            self.__ansible.fail_json(msg=error_message)
+
+        return reordered_configlets_list
 
     def __refresh_user_inventory(self, user_inventory: DeviceInventory):
         """
@@ -1137,6 +1157,7 @@ def get_device_configlets(self, device_lookup: str):
                 )
                 for configlet in configlets_data:
                     configlet_list.append(CvElement(cv_data=configlet))
+                MODULE_LOGGER.debug("configlet_list in get_device_configlets function is: %s", str([x.name for x in configlet_list]))
                 return configlet_list
         return None
 
@@ -1926,6 +1947,7 @@ def detach_configlets(self, user_inventory: DeviceInventory):
         for device in user_inventory.devices:
             result_data = CvApiResult(action_name=device.fqdn + "_configlet_removed")
             # FIXME: Should we ignore devices listed with no configlets ?
+            MODULE_LOGGER.debug("Device configlet list is: %s", str(device.configlets))
             if device.configlets is not None:
                 # get device facts from CV
                 device_facts = self.get_device_facts(
@@ -1937,19 +1959,19 @@ def detach_configlets(self, user_inventory: DeviceInventory):
                     self.__get_configlet_list_inherited_from_container(device)
                     + device.configlets
                 )
-
+                MODULE_LOGGER.debug("Expected configlet list is: %s", str(expected_device_configlet_list))
                 configlets_to_remove = []
 
                 # get list of configured configlets
                 configlets_attached = self.get_device_configlets(
                     device_lookup=device.info[self.__search_by]
                 )
                 MODULE_LOGGER.debug(
-                    "Current configlet attached {0}".format(
+                    "Current configlet attached lru cache {0}".format(
                         [x.name for x in configlets_attached]
                     )
                 )
-
+                MODULE_LOGGER.debug("Configlets attached raw data {0}".format(configlets_attached))
                 # For each configlet not in the list, add to list of configlets to remove
                 for configlet in configlets_attached:
                     if configlet.name not in expected_device_configlet_list:

ansible_collections/arista/cvp/plugins/module_utils/generic_tools.py

@@ -55,6 +55,19 @@ def key(self):
         if 'key' in self.__cv_data:
             return self.__cv_data['key']
 
+    @property
+    def reconciled(self):
+        """
+        key Getter to expose RECONCILED field
+
+        Returns
+        -------
+        str
+            Value of the KEY field
+        """
+        if 'reconciled' in self.__cv_data:
+            return self.__cv_data['reconciled']
+
     @property
     def data(self):
         """