Vlans and interfaces resource modules check and diff modes support (#301

)

* Vlans and interfaces resource modules check and diff modes support

* Add fragment file

* Fix sanity error

* Fix sanity error

* Fix sanity error

* Fix sanity error

* Fix sanity error

* Fix sanity error

* Fix sanity error

* Change unit test script

* Fix unit test error

* Add method line back

* Fix sanity error

changelogs/fragments/301-playbook-check-diff-modes-for-vlans-interfaces.yaml

@@ -0,0 +1,2 @@
+major_changes:
+        - vlans_interfaces - Playbook check and diff modes supports for sonic_vlans and sonic_interfaces modules (https://github.com/ansible-collections/dellemc.enterprise_sonic/pull/301).

plugins/module_utils/network/sonic/config/interfaces/interfaces.py

@@ -18,6 +18,14 @@
 except ImportError:
     from urllib.parse import quote
 
+"""
+The use of natsort causes sanity error due to it is not available in python version currently used.
+When natsort becomes available, the code here and below using it will be applied.
+from natsort import (
+    natsorted,
+    ns
+)
+"""
 from ansible_collections.ansible.netcommon.plugins.module_utils.network.common.cfg.base import (
     ConfigBase,
 )
@@ -39,10 +47,19 @@
     update_states,
     normalize_interface_name
 )
+from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.utils.formatted_diff_utils import (
+    __DELETE_CONFIG_IF_NO_SUBCONFIG,
+    get_new_config,
+    get_formatted_config_diff
+)
 from ansible.module_utils._text import to_native
 from ansible.module_utils.connection import ConnectionError
 import traceback
 
+TEST_KEYS_formatted_diff = [
+    {'config': {'name': '', '__delete_op': __DELETE_CONFIG_IF_NO_SUBCONFIG}},
+]
+
 LIB_IMP_ERR = None
 ERR_MSG = None
 try:
@@ -118,6 +135,22 @@ def execute_module(self):
         if result['changed']:
             result['after'] = changed_interfaces_facts
 
+        new_config = changed_interfaces_facts
+        old_config = existing_interfaces_facts
+        if self._module.check_mode:
+            result.pop('after', None)
+            new_config = get_new_config(commands, existing_interfaces_facts,
+                                        TEST_KEYS_formatted_diff)
+            # See the above comment about natsort module
+            # new_config = natsorted(new_config, key=lambda x: x['name'])
+            # For time-being, use simple "sort"
+            new_config.sort(key=lambda x: x['name'])
+            result['after(generated)'] = new_config
+            old_config.sort(key=lambda x: x['name'])
+
+        if self._module._diff:
+            result['config_diff'] = get_formatted_config_diff(old_config,
+                                                              new_config)
         result['warnings'] = warnings
         return result
 
@@ -173,10 +206,19 @@ def _state_replaced(self, want, have, diff):
                   to the desired configuration
         """
         commands = self.filter_comands_to_change(diff, have)
-        requests = self.get_delete_interface_requests(commands, have)
-        requests.extend(self.get_modify_interface_requests(commands, have))
+        commands_del = self.filter_comands_to_delete(commands, have)
+        requests = self.get_delete_interface_requests(commands_del, have)
+        commands_mer = self.filter_comands_to_change(commands, have)
+        requests.extend(self.get_modify_interface_requests(commands_mer, have))
         if commands and len(requests) > 0:
-            commands = update_states(commands, "replaced")
+            commands_dlt, commands_rep = self.classify_delete_commands(commands_del)
+            commands = []
+            if commands_dlt:
+                commands.extend(update_states(commands_dlt, "deleted"))
+            if commands_rep:
+                commands.extend(update_states(commands_rep, "replaced"))
+            if commands_mer:
+                commands.extend(update_states(commands_mer, "replaced"))
         else:
             commands = []
 
@@ -192,14 +234,18 @@ def _state_overridden(self, want, have, diff):
                   to the desired configuration
         """
         commands = []
-        commands_del = self.filter_comands_to_change(want, have)
+        commands_chg = self.filter_comands_to_change(want, have)
+        commands_del = self.filter_comands_to_delete(commands_chg, have)
         requests = self.get_delete_interface_requests(commands_del, have)
         del_req_count = len(requests)
         if commands_del and del_req_count > 0:
-            commands_del = update_states(commands_del, "deleted")
-            commands.extend(commands_del)
+            commands_dlt, commands_ovr = self.classify_delete_commands(commands_del)
+            if commands_dlt:
+                commands.extend(update_states(commands_dlt, "deleted"))
+            if commands_ovr:
+                commands.extend(update_states(commands_ovr, "overridden"))
 
-        commands_over = diff
+        commands_over = self.filter_comands_to_change(diff, have)
         requests.extend(self.get_modify_interface_requests(commands_over, have))
         if commands_over and len(requests) > del_req_count:
             commands_over = update_states(commands_over, "overridden")
@@ -216,7 +262,7 @@ def _state_merged(self, want, have, diff):
         :returns: the commands necessary to merge the provided into
                   the current configuration
         """
-        commands = diff
+        commands = self.filter_comands_to_change(diff, have)
         requests = self.get_modify_interface_requests(commands, have)
         if commands and len(requests) > 0:
             commands = update_states(commands, "merged")
@@ -241,10 +287,16 @@ def _state_deleted(self, want, have, diff):
         else:
             commands = want
 
-        requests = self.get_delete_interface_requests(commands, have)
+        commands_del = self.filter_comands_to_delete(commands, have)
+        requests = self.get_delete_interface_requests(commands_del, have)
 
-        if commands and len(requests) > 0:
-            commands = update_states(commands, "deleted")
+        if commands_del and len(requests) > 0:
+            commands_dlt, commands_mer = self.classify_delete_commands(commands_del)
+            commands = []
+            if commands_dlt:
+                commands.extend(update_states(commands_dlt, "deleted"))
+            if commands_mer:
+                commands.extend(update_states(commands_mer, "merged"))
         else:
             commands = []
 
@@ -255,15 +307,30 @@ def filter_comands_to_delete(self, configs, have):
 
         for conf in configs:
             if self.is_this_delete_required(conf, have):
+                intf_name = conf['name']
+
                 temp_conf = dict()
                 temp_conf['name'] = conf['name']
-                temp_conf['description'] = ''
-                temp_conf['mtu'] = 9100
-                temp_conf['enabled'] = True
-                temp_conf['speed'] = 'SPEED_DEFAULT'
-                temp_conf['auto_negotiate'] = False
-                temp_conf['fec'] = 'FEC_DISABLED'
-                temp_conf['advertised_speed'] = ''
+                if intf_name == 'Management0':
+                    temp_conf['description'] = 'Management0'
+                    temp_conf['mtu'] = 1500
+                    temp_conf['enabled'] = True
+                    temp_conf['speed'] = None
+                    temp_conf['auto_negotiate'] = None
+                    temp_conf['fec'] = None
+                else:
+                    temp_conf['description'] = ''
+                    temp_conf['mtu'] = 9100
+                    temp_conf['enabled'] = False
+                    if intf_name.startswith('Eth'):
+                        temp_conf['speed'] = self.retrieve_default_intf_speed(conf['name'])
+                        temp_conf['auto_negotiate'] = False
+                        temp_conf['fec'] = 'FEC_DISABLED'
+                    else:
+                        temp_conf['speed'] = None
+                        temp_conf['auto_negotiate'] = None
+                        temp_conf['fec'] = None
+                temp_conf['advertised_speed'] = None
                 commands.append(temp_conf)
         return commands
 
@@ -277,15 +344,11 @@ def filter_comands_to_change(self, configs, have):
 
     def get_modify_interface_requests(self, configs, have):
         self.delete_flag = False
-        commands = self.filter_comands_to_change(configs, have)
-
-        return self.get_interface_requests(commands, have)
+        return self.get_interface_requests(configs, have)
 
     def get_delete_interface_requests(self, configs, have):
         self.delete_flag = True
-        commands = self.filter_comands_to_delete(configs, have)
-
-        return self.get_interface_requests(commands, have)
+        return self.get_interface_requests(configs, have)
 
     def get_interface_requests(self, configs, have):
         requests = []
@@ -341,17 +404,16 @@ def retrieve_default_intf_speed(self, intf_name):
                 self._module.fail_json(msg=str(exc), code=exc.code)
 
         # Read the speed
+        intf_speed = 'SPEED_DEFAULT'
         method = GET
         request = {"path": eth_url, "method": method}
         try:
             response = edit_config(self._module, to_request(self._module, request))
-        except ConnectionError as exc:
-            self._module.fail_json(msg=str(exc), code=exc.code)
-
-        intf_speed = 'SPEED_DEFAULT'
-        if "openconfig-if-ethernet:port-speed" in response[0][1]:
-            speed_str = response[0][1].get("openconfig-if-ethernet:port-speed", '')
-            intf_speed = speed_str.split(":", 1)[-1]
+            if "openconfig-if-ethernet:port-speed" in response[0][1]:
+                speed_str = response[0][1].get("openconfig-if-ethernet:port-speed", '')
+                intf_speed = speed_str.split(":", 1)[-1]
+        except Exception as exc:
+            pass
 
         return intf_speed
 
@@ -362,7 +424,7 @@ def is_this_delete_required(self, conf, have):
         if intf:
             if (intf['name'].startswith('Loopback') or
                 not ((intf.get('description') is None or intf.get('description') == '') and
-                     (intf.get('enabled') is None or intf.get('enabled') is True) and
+                     (intf.get('enabled') is None or intf.get('enabled') is False) and
                      (intf.get('mtu') is None or intf.get('mtu') == 9100) and
                      (intf.get('fec') is None or intf.get('fec') == 'FEC_DISABLED') and
                      (intf.get('speed') is None or
@@ -468,3 +530,19 @@ def build_create_autoneg_request(self, conf):
                 request = {"path": eth_url, "method": method, "data": payload}
 
         return request
+
+    def classify_delete_commands(self, configs):
+        commands_del = []
+        commands_mer = []
+
+        if not configs:
+            return commands_del, commands_mer
+
+        for conf in configs:
+            name = conf["name"]
+            if name.startswith('Loopback'):
+                commands_del.append(conf)
+            else:
+                commands_mer.append(conf)
+
+        return commands_del, commands_mer

plugins/module_utils/network/sonic/config/vlans/vlans.py

@@ -35,12 +35,20 @@
     to_request,
     edit_config
 )
+from ansible_collections.dellemc.enterprise_sonic.plugins.module_utils.network.sonic.utils.formatted_diff_utils import (
+    __DELETE_CONFIG_IF_NO_SUBCONFIG,
+    get_new_config,
+    get_formatted_config_diff
+)
 from ansible.module_utils.connection import ConnectionError
 
 
 TEST_KEYS = [
     {'config': {'vlan_id': ''}},
 ]
+TEST_KEYS_formatted_diff = [
+    {'config': {'vlan_id': '', '__delete_op': __DELETE_CONFIG_IF_NO_SUBCONFIG}},
+]
 
 
 class Vlans(ConfigBase):
@@ -98,6 +106,17 @@ def execute_module(self):
         if result['changed']:
             result['after'] = changed_vlans_facts
 
+        new_config = changed_vlans_facts
+        if self._module.check_mode:
+            result.pop('after', None)
+            new_config = get_new_config(commands, existing_vlans_facts,
+                                        TEST_KEYS_formatted_diff)
+            new_config.sort(key=lambda x: x['vlan_id'])
+            result['after(generated)'] = new_config
+
+        if self._module._diff:
+            result['config_diff'] = get_formatted_config_diff(existing_vlans_facts,
+                                                              new_config)
         result['warnings'] = warnings
         return result
 

tests/unit/modules/network/sonic/fixtures/sonic_interfaces.yaml

@@ -45,33 +45,36 @@ deleted_01:
         value:
           openconfig-interfaces:interfaces:
             interface:
-              - name: Eth1/1
+              - name: 'Eth1/1'
                 config: 
                   enabled: false
                   description: 'Test Desc for eth1/1'
-                  mtu: 1600
-              - name: Loopback1
+                  mtu: 8888
+              - name: 'Loopback123'
                 config: 
                   enabled: false
-                  description: 'Test Desc for eth1/1'
+                  description: 'Test Desc for Loopback123'
   expected_config_requests:
     - path: "data/openconfig-interfaces:interfaces/interface=Eth1%2F1/config"
       method: "patch"
       data:
         openconfig-interfaces:config:
-          enabled: true
+          enabled: false
           description: ''
           mtu: 9100
-    - path: "data/openconfig-interfaces:interfaces/interface=Loopback1"
+    - path: "data/openconfig-interfaces:interfaces/interface=Eth1%2F1/openconfig-if-ethernet:ethernet/config/port-speed"
+      method: "delete"
+    - path: "data/openconfig-interfaces:interfaces/interface=Eth1%2F1/openconfig-if-ethernet:ethernet/config/port-speed"
+      method: "get"
+    - path: "data/openconfig-interfaces:interfaces/interface=Loopback123"
       method: "delete"
-      data:
 
 replaced_01:
   module_args:
     state: replaced
     config:
       - name: Eth1/1
-        mtu: 1600      
+        mtu: 5555      
   existing_interfaces_config:
     - path: "data/openconfig-interfaces:interfaces"
       response:
@@ -89,14 +92,18 @@ replaced_01:
       method: "patch"
       data:
         openconfig-interfaces:config:
-          enabled: true
+          enabled: false
           description: ''
           mtu: 9100
     - path: "data/openconfig-interfaces:interfaces/interface=Eth1%2F1/config"
       method: "patch"
       data:
         openconfig-interfaces:config:
-          mtu: 1600
+          mtu: 5555
+    - path: "data/openconfig-interfaces:interfaces/interface=Eth1%2F1/openconfig-if-ethernet:ethernet/config/port-speed"
+      method: "delete"
+    - path: "data/openconfig-interfaces:interfaces/interface=Eth1%2F1/openconfig-if-ethernet:ethernet/config/port-speed"
+      method: "get"
 
 overridden_01:
   module_args: