[DPB]: Shut down interface before dynamic port breakout (#1303)

Changes:
-- Shutdown the interfaces after config validation while Dy Port Breakout.
-- Validate del ports before calling breakOutPorts API.

Signed-off-by: Praveen Chaudhary [email protected]

Fixes sonic-net/sonic-buildimage#6646, sonic-net/sonic-buildimage#6631,

Signed-off-by: Praveen Chaudhary [email protected]

Author: Praveen Chaudhary

config/config_mgmt.py

@@ -369,9 +369,12 @@ def breakOutPort(self, delPorts=list(), portJson=dict(), force=False, \
             if_name_map, if_oid_map = port_util.get_interface_oid_map(dataBase)
             self.sysLog(syslog.LOG_DEBUG, 'if_name_map {}'.format(if_name_map))
 
-            # If we are here, then get ready to update the Config DB, Update
-            # deletion of Config first, then verify in Asic DB for port deletion,
-            # then update addition of ports in config DB.
+            # If we are here, then get ready to update the Config DB as below:
+            # -- shutdown the ports,
+            # -- Update deletion of ports in Config DB,
+            # -- verify Asic DB for port deletion,
+            # -- then update addition of ports in config DB.
+            self._shutdownIntf(delPorts)
             self.writeConfigDB(delConfigToLoad)
             # Verify in Asic DB,
             self._verifyAsicDB(db=dataBase, ports=delPorts, portMap=if_name_map, \
@@ -507,6 +510,27 @@ def _addPorts(self, portJson=dict(), loadDefConfig=True):
 
         return configToLoad, True
 
+    def _shutdownIntf(self, ports):
+        """
+        Based on the list of Ports, create a dict to shutdown port, update Config DB.
+        Shut down all the interfaces before deletion.
+
+        Parameters:
+            ports(list): list of ports, which are getting deleted due to DPB.
+
+        Returns:
+            void
+        """
+        shutDownConf = dict(); shutDownConf["PORT"] = dict()
+        for intf in ports:
+            shutDownConf["PORT"][intf] = {"admin_status": "down"}
+        self.sysLog(msg='shutdown Interfaces: {}'.format(shutDownConf))
+
+        if len(shutDownConf["PORT"]):
+            self.writeConfigDB(shutDownConf)
+
+        return
+
     def _mergeConfigs(self, D1, D2, uniqueKeys=True):
         '''
         Merge D2 dict in D1 dict, Note both first and second dict will change.

config/main.py

@@ -114,32 +114,6 @@ def _get_breakout_options(ctx, args, incomplete):
             all_mode_options = [str(c) for c in breakout_mode_options if incomplete in c]
         return all_mode_options
 
-def shutdown_interfaces(ctx, del_intf_dict):
-    """ shut down all the interfaces before deletion """
-    for intf in del_intf_dict:
-        config_db = ctx.obj['config_db']
-        if clicommon.get_interface_naming_mode() == "alias":
-            interface_name = interface_alias_to_name(config_db, intf)
-            if interface_name is None:
-                click.echo("[ERROR] interface name is None!")
-                return False
-
-        if interface_name_is_valid(config_db, intf) is False:
-            click.echo("[ERROR] Interface name is invalid. Please enter a valid interface name!!")
-            return False
-
-        port_dict = config_db.get_table('PORT')
-        if not port_dict:
-            click.echo("port_dict is None!")
-            return False
-
-        if intf in port_dict:
-            config_db.mod_entry("PORT", intf, {"admin_status": "down"})
-        else:
-            click.secho("[ERROR] Could not get the correct interface name, exiting", fg='red')
-            return False
-    return True
-
 def _validate_interface_mode(ctx, breakout_cfg_file, interface_name, target_brkout_mode, cur_brkout_mode):
     """ Validate Parent interface and user selected mode before starting deletion or addition process """
     breakout_file_input = readJsonFile(breakout_cfg_file)["interfaces"]
@@ -3181,12 +3155,7 @@ def breakout(ctx, interface_name, mode, verbose, force_remove_dependencies, load
     del_intf_dict = {intf: del_ports[intf]["speed"] for intf in del_ports}
 
     if del_intf_dict:
-        """ shut down all the interface before deletion """
-        ret = shutdown_interfaces(ctx, del_intf_dict)
-        if not ret:
-            raise click.Abort()
         click.echo("\nPorts to be deleted : \n {}".format(json.dumps(del_intf_dict, indent=4)))
-
     else:
         click.secho("[ERROR] del_intf_dict is None! No interfaces are there to be deleted", fg='red')
         raise click.Abort()
@@ -3213,6 +3182,12 @@ def breakout(ctx, interface_name, mode, verbose, force_remove_dependencies, load
         del_intf_dict.pop(item)
         add_intf_dict.pop(item)
 
+    # validate all del_ports before calling breakOutPort
+    for intf in del_intf_dict.keys():
+        if not interface_name_is_valid(config_db, intf):
+            click.secho("[ERROR] Interface name {} is invalid".format(intf))
+            raise click.Abort()
+
     click.secho("\nFinal list of ports to be deleted : \n {} \nFinal list of ports to be added :  \n {}".format(json.dumps(del_intf_dict, indent=4), json.dumps(add_intf_dict, indent=4), fg='green', blink=True))
     if not add_intf_dict:
         click.secho("[ERROR] add_intf_dict is None or empty! No interfaces are there to be added", fg='red')

tests/config_mgmt_test.py

@@ -78,6 +78,41 @@ def test_break_out(self):
         self.dpb_port4_4x25G_2x50G_f_l(curConfig)
         return
 
+    def test_shutdownIntf_call(self):
+        '''
+        Verify that _shutdownIntf() is called with deleted ports while calling
+        breakOutPort()
+        '''
+        curConfig = deepcopy(configDbJson)
+        cmdpb = self.config_mgmt_dpb(curConfig)
+
+        # create ARGS
+        dPorts, pJson = self.generate_args(portIdx=8, laneIdx=73, \
+            curMode='1x50G(2)+2x25G(2)', newMode='2x50G')
+
+        # Try to breakout and see if _shutdownIntf is called
+        deps, ret = cmdpb.breakOutPort(delPorts=dPorts, portJson=pJson, \
+            force=True, loadDefConfig=False)
+
+        # verify correct function call to writeConfigDB after _shutdownIntf()
+        assert cmdpb.writeConfigDB.call_count == 3
+        print(cmdpb.writeConfigDB.call_args_list[0])
+        (args, kwargs) = cmdpb.writeConfigDB.call_args_list[0]
+        print(args)
+
+        # in case of tuple also, we should have only one element
+        if type(args) == tuple:
+            args = args[0]
+        assert "PORT" in args
+
+        # {"admin_status": "down"} should be set for all ports in dPorts
+        assert len(args["PORT"]) == len(dPorts)
+        # each port should have {"admin_status": "down"}
+        for port in args["PORT"].keys():
+            assert args["PORT"][port]['admin_status'] == 'down'
+
+        return
+
     def tearDown(self):
         try:
             os.remove(config_mgmt.CONFIG_DB_JSON_FILE)
@@ -229,7 +264,7 @@ def checkResult(self, cmdpb, delConfig, addConfig):
             void
         '''
         calls = [mock.call(delConfig), mock.call(addConfig)]
-        assert cmdpb.writeConfigDB.call_count == 2
+        assert cmdpb.writeConfigDB.call_count == 3
         cmdpb.writeConfigDB.assert_has_calls(calls, any_order=False)
         return
 
@@ -497,7 +532,6 @@ def dpb_port8_4x25G_2x50G_f_l(self, curConfig):
                 }
             }
         }
-        assert cmdpb.writeConfigDB.call_count == 2
         self.checkResult(cmdpb, delConfig, addConfig)
         self.postUpdateConfig(curConfig, delConfig, addConfig)
         return