Addressed the code review comments

 1) Reused the existing Python functions
 2) Modified the function names to use snake case
 3) Changed the print to logger.log_info
 4) Fixed the indentation
 5) Modified it to work for both single and multi asic
 6) Reset the env variable
 7) When user issues TSB when the service is running, stop the service

Signed-off-by: saksarav <[email protected]>

Author: saksarav-nokia

dockers/docker-fpm-frr/base_image_files/TSB

@@ -21,6 +21,15 @@ then
   sudo config mux mode auto all
 fi
 
+if [ -z "$STARTED_BY_TSA_TSB_SERVICE" ]; then
+  service='startup_tsa_tsb.service'
+  if [[ $(/bin/systemctl show $service --property ActiveState  --value) == "active" ]] && \
+     [[ $(/bin/systemctl show $service --property SubState  --value) == "running" ]]; then
+      echo "Stopping $service before configuring TSB"
+      systemctl stop $service
+  fi
+fi
+
 /usr/bin/TS TSB
 if [[ "$(sonic-cfggen -d -v DEVICE_METADATA.localhost.type)" == *"SpineRouter"* ]] ; then
   if [[ "$1" != "chassis" ]] ; then

files/scripts/startup_tsa_tsb.py

@@ -3,146 +3,152 @@
 # Name: startup_tsa_tsb.py, version: 1.0
 #
 # Description: Module contains the definitions to the VOQ Startup TSA-TSB service
-
+from sonic_py_common import multi_asic, device_info
+from sonic_py_common.logger import Logger
 import subprocess
 import sys, getopt
 from threading import Timer
-import os.path
-
-def getPlatform():
-    platform_key = "DEVICE_METADATA['localhost']['platform']"
-    platform = (subprocess.check_output(['sonic-cfggen', '-d', '-v', platform_key.replace('"',"'")]).strip()).decode()
-    return platform
+import os
 
-def getNumAsics():
-    platform = getPlatform()
-    asic_config_file = '/usr/share/sonic/device/{}/asic.conf'.format(platform)
-    file = open(asic_config_file, 'r')
-    Lines = file.readlines()
-    for line in Lines:
-       field = line.split('=')[0].strip()
-       if field == "NUM_ASIC":
-         return line.split('=')[1].strip()
-    return 0
+# Global Logger class instance
+logger = Logger("startup_tsa_tsb")
+logger.set_min_log_priority_info()
 
-def getTsbTimerInterval():
-    platform = getPlatform()
+def get_tsb_timer_interval():
+    platform = device_info.get_platform()
     conf_file = '/usr/share/sonic/device/{}/startup-tsa-tsb.conf'.format(platform)
     file = open(conf_file, 'r')
     Lines = file.readlines()
     for line in Lines:
-       field = line.split('=')[0].strip()
-       if field == "STARTUP_TSB_TIMER":
-         return line.split('=')[1].strip()
+        field = line.split('=')[0].strip()
+        if field == "STARTUP_TSB_TIMER":
+            return line.split('=')[1].strip()
     return 0
 
-def getSonicConfig(ns, config_name):
-    return subprocess.check_output(['sonic-cfggen', '-d', '-v', config_name.replace('"', "'"), '-n', ns.replace('"', "'")]).strip()
+def get_sonic_config(ns, config_name):
+    if ns == "":
+        return subprocess.check_output(['sonic-cfggen', '-d', '-v', config_name.replace('"', "'"), ]).strip()
+    else:
+        return subprocess.check_output(['sonic-cfggen', '-d', '-v', config_name.replace('"', "'"), '-n', ns.replace('"', "'")]).strip()
 
-def getSubRole(asic_ns):
+def get_sub_role(asic_ns):
     sub_role_config = "DEVICE_METADATA['localhost']['sub_role']"
-    sub_role = (getSonicConfig(asic_ns, sub_role_config)).decode()
+    sub_role = (get_sonic_config(asic_ns, sub_role_config)).decode()
     return sub_role
 
-def getTsaConfig(asic_ns):
+def get_tsa_config(asic_ns):
     tsa_config = 'BGP_DEVICE_GLOBAL.STATE.tsa_enabled'
-    tsa_ena = (getSonicConfig(asic_ns, tsa_config)).decode()
-    print('{}: {} - CONFIG_DB.{} : {}'.format(__file__, asic_ns, tsa_config, tsa_ena))
+    tsa_ena = (get_sonic_config(asic_ns, tsa_config)).decode()
+    if asic_ns == "":
+        logger.log_info('CONFIG_DB.{} : {}'.format(tsa_config, tsa_ena))
+    else:
+        logger.log_info('{} - CONFIG_DB.{} : {}'.format(asic_ns, tsa_config, tsa_ena))
     return tsa_ena
 
-def get_tsa_status():
-    asic_num = getNumAsics()
-    counter = 0
-    for asic_id in range(int(asic_num)):
-       asic_ns = 'asic{}'.format(asic_id)
-       sub_role = getSubRole(asic_ns)
-       if sub_role == 'FrontEnd':
-          tsa_enabled = getTsaConfig(asic_ns)
-          if tsa_enabled == 'false':
-             counter += 1
-    if counter == int(asic_num):
-       return True;
+def get_tsa_status(num_asics):
+    if num_asics > 1:
+        counter = 0
+        for asic_id in range(int(asic_num)):
+            asic_ns = 'asic{}'.format(asic_id)
+            sub_role = get_sub_role(asic_ns)
+            if sub_role == 'FrontEnd':
+                tsa_enabled = get_tsa_config(asic_ns)
+                if tsa_enabled == 'false':
+                    counter += 1
+        if counter == int(asic_num):
+            return True;
     else:
-       return False;
+        tsa_enabled = get_tsa_config("")
+        if tsa_enabled == 'false':
+            return True;
+    return False;
 
 def config_tsa():
-    tsa_ena = get_tsa_status()
+    num_asics = multi_asic.get_num_asics()
+    tsa_ena = get_tsa_status(num_asics)
     if tsa_ena == True:
-       print("{}: Configuring TSA".format(__file__))
-       subprocess.check_output(['TSA']).strip()
+        logger.log_info("Configuring TSA")
+        subprocess.check_output(['TSA']).strip()
     else:
-        print("{}: Either TSA is already configured or switch sub_role is not Frontend - not configuring TSA".format(__file__))
+        if num_asics > 1:
+            logger.log_info("Either TSA is already configured or switch sub_role is not Frontend - not configuring TSA")
+        else:
+            logger.log_info("Either TSA is already configured - not configuring TSA")
     return tsa_ena
 
 def config_tsb():
-    print("startup_tsa_tsb: Configuring TSB")
+    logger.log_info("startup_tsa_tsb: Configuring TSB")
     subprocess.check_output(['TSB']).strip()
     tsb_issued = True
     return
 
 def start_tsb_timer(interval):
     global timer
-    print("{}: Starting timer with interval {} seconds to configure TSB".format(__file__, interval))
+    logger.log_info("Starting timer with interval {} seconds to configure TSB".format(interval))
     timer = Timer(int(interval), config_tsb)
     timer.start()
     timer.join()
     return
 
 def print_usage():
-    print ("Usage: startup_tsa_tsb.py [options] command")
-    print ("options:")
-    print("  -h | --help       : this help message")
-    print("command:")
-    print("start     : start the TSA/TSB")
-    print("stop      : stop the TSA/TSB")
+    logger.log_info("Usage: startup_tsa_tsb.py [options] command")
+    logger.log_info("options:")
+    logger.log_info("  -h | --help       : this help message")
+    logger.log_info("command:")
+    logger.log_info("start     : start the TSA/TSB")
+    logger.log_info("stop      : stop the TSA/TSB")
     return
 
-def start_tsa_tsb(timer):
+def reset_env_variables():
+    logger.log_info("Resetting environment variable")
+    os.environ.pop('STARTED_BY_TSA_TSB_SERVICE')
+    return
 
+def start_tsa_tsb(timer):
     #Configure TSA if it was not configured already in CONFIG_DB
     tsa_enabled = config_tsa()
     if tsa_enabled == True:
-      #Start the timer to configure TSB
-      start_tsb_timer(timer)
+        #Start the timer to configure TSB
+        start_tsb_timer(timer)
     return
 
 def stop_tsa_tsb():
-    #for future use
+    reset_env_variables()
     return
 
 def main():
-    platform = getPlatform()
+    platform = device_info.get_platform()
     conf_file = '/usr/share/sonic/device/{}/startup-tsa-tsb.conf'.format(platform)
     #This check should be moved to service file or make this feature as configurable.
     #Adding it here for now.
     if not os.path.exists(conf_file):
-       print ("{} does not exist, exiting the service".format(conf_file))
-       return
+        logger.log_info("{} does not exist, exiting the service".format(conf_file))
+        return
     if len(sys.argv) <= 1:
-       print_usage()
-       return
+        print_usage()
+        return
 
     # parse command line options:
     try:
-       opts, args = getopt.getopt(sys.argv[1:], 'h:', ['help' ])
+        opts, args = getopt.getopt(sys.argv[1:], 'h:', ['help' ])
     except getopt.GetoptError:
-       print_usage()
-       return
+        print_usage()
+        return
 
     for opt, arg in opts:
-       if opt in ("-h", "--help"):
-          print_usage()
-          return
+        if opt in ("-h", "--help"):
+            print_usage()
+            return
 
     for arg in args:
         if arg == 'start':
-           tsb_timer = getTsbTimerInterval()
-           start_tsa_tsb(tsb_timer)
+            tsb_timer = get_tsb_timer_interval()
+            start_tsa_tsb(tsb_timer)
         elif arg == 'stop':
-           stop_tsa_tsb()
+            stop_tsa_tsb()
         else:
-           print_usage()
-           return
+            print_usage()
+            return
 
     return