4
\$\begingroup\$

This is a python (2.7.13 according to sys.version) script that reads systemd service names from a file and gets their CPU and Memory usage. It does that by first getting the main PID of the service by either searching for it in /var/run (most services have a pidfile containing the main PID there, however, there are different ways in which they are created -- in a subfolder or not, with a specific name and etc., but almost all of them have the .pid extension) or if that fails, it just greps it. The first way isn't something that is the same for all services, but I put effort to do it, because it is a more optimized and a faster way to gather that information, while reading from an external grep is a hassle. After getting the PID I get all child processes and calculate their CPU usage from /proc stat files (this is the same thing that psutil does, I could just use it for CPU usage, but since I know the way and it works -- it is faster manually implemented, or is it..). For the memory usage I just skipped that part and directly used psutil. I would really like to improve the formatting and readability of my code, as well as its performance. And I do have a few questions like:

  1. Is it a good idea to make all those checks for a pidfile in /var/run just to save time from external grepping?

  2. Are all those nested ifs there along with the try / except on top of them a good style, and if not, how would I be able to replace them?

  3. Is it a bad practice to manually implement the gathering of information about CPU usage, while still using an external library for memory usage?

Any suggestions about formatting, styling, readability, optimization, etcetera are welcome. The script runs relatively fast (~0.8s/10services) on my machine, however, I have an SSD, and assuming it could run on a computer with an HDD or even slower CPUs the execution time could be a lot higher, especially if they are 100 services and not 10.

#!/usr/bin/env python

# Exit codes:
# 6 :   Getopt Error. Probably wrong argument or misspell.
# 7 :   Global exception caught. Could be anything.

# Import default system libraries.

import getopt
import os
import subprocess
import sys
# from datetime import datetime # Uncomment if going to benchmark for speed.

# Import external libraries.

import psutil
from pathlib import Path

# startTime = datetime.now() # Start the timer for benchmarking. Must uncomment last line as well.

# Get arguments and set configuration.

def parse_args():
    cfgfile = 'smon.conf'

    if len(sys.argv) > 1:
        try:
            opts, args = getopt.getopt(sys.argv[1:], 'c:', ['config='])
        except getopt.GetoptError:
            print("An error occured while parsing your arguments. Check the proper usage of the script.")
            sys.exit(6)

        for opt, arg in opts:
            if opt in ('-c', '--config'):
                cfgfile = str(arg)

    return cfgfile

# Read services from the configuration file and add them into a list.

def load_services(handlerlist, cfg):
    try:
        with open(cfg, "r") as servfile:
            for line in servfile:
                handlerlist.append(line.strip())
    except:
        print("The file {} most probably does not exist. ".format(cfg))

    return handlerlist

# Read CPU and Memory usage of the processes.

def read_stats(ss):
    cpud = {}
    memd = {}

    for pid in ss:

        with open(os.path.join('/proc/', str(pid), 'stat'), 'r') as pfile: 
            pidtimes = pfile.read().split(' ')
            pname = str(pidtimes[1])[1:-1]

        cpud[pname] = '0'
        memd[pname] = '0'

    for pid in ss:

        # CPU times and usage can be found in the /proc/ filesystem in stat files.

        with open(os.path.join('/proc/', str(pid), 'stat'), 'r') as pfile: 
            pidtimes = pfile.read().split(' ')
            pname = str(pidtimes[1])[1:-1]
            utime = int(pidtimes[13]) # utime is the 14th element in the stat file (man proc).
            stime = int(pidtimes[14]) # stime is the 15th element in the stat file (man proc).
            pidtotal = utime - stime

        with open('/proc/stat', 'r') as cfile: # Get total system CPU times.
            cputimes = cfile.readline().split(' ')
            cputotal = 0
            for integ in cputimes[2:]:
                integ = int(integ)
                cputotal = cputotal + integ

        usg = (pidtotal / cputotal) * 100 # Process CPU usage is process cpu times / system cpu time.

        if usg < 0: # Deny negative values
            usg = 0

        newusg = int(cpud[pname]) + usg
        cpud[pname] = str(newusg) # Calculate the usage and add to it.
        phandler = psutil.Process(pid) # Generate a process class for the given PID.
        pmem = phandler.memory_percent() # Get memory usage in percents of services.
        newpmem = float(memd[pname]) + pmem
        memd[pname] = str(newpmem)

    return cpud, memd

# Get the Process ID for each service in the configuration file. 

def get_pid(slist):
    pidchecks = [] # Predefine the list of PIDs.

    for svc in slist:

        cpuusage = 0 # Predefine the variable for CPU usage.

        try: # For every service, try to find its PID file in /var/run and read it.
            pidfpath = '/var/run/{}/{}.pid'.format(svc, svc)
            if not Path(pidfpath).exists(): # Most services have a /var/run/service/service.pid file.
                pidfpath = '/var/run/{}.pid'.format(svc)
                if not Path(pidfpath).exists(): # Some services use 'd' after their names for daemon.
                    pidfpath = '/var/run/{}.pid'.format(svc + 'd')
                    if not Path(pidfpath).exists(): # Others have a /var/run/service.pid file.
                        pidfolder = '/var/run/{}'.format(svc)
                        tmpc = os.listdir(pidfolder)
                        for f in tmpc: # And others have various pidfiles like /var/run/service/pid.
                            f = str(f)
                            if 'pid' in f:
                                pidfpath = pidfolder + '/' + f # Add the file to the dir path. 

            with open(pidfpath, 'r') as pidf:
                mainpid = int(pidf.readline().strip()) # Read the PID number. Not sure if strip is needed. Have to check.

        except Exception as e: # If such a PID file does not exist, get Main PID from parsing systemctl.
            try:
                mainpid = int(subprocess.check_output("systemctl status {} | grep 'Main PID: ' | grep -Eo '[[:digit:]]*' | head -n 1".format(svc), shell=True))
            except ValueError as e: # If systemctl returns nothing, then such a service does not exist.
                pass

        try: # Get all the children of the Main PID and append them to a list.
            mainproc = psutil.Process(mainpid)
            mchildren = mainproc.children(recursive=True)
            pidchecks.append(mainpid)
            for child in mchildren:
                pidchecks.append(child.pid)
        except psutil._exceptions.NoSuchProcess: # Return an error if there is no such process working.
            print("No running process with pid {} ({}). Probably the service isn't working.\n".format(str(mainpid), svc))
        except psutil._exceptions.ZombieProcess: # Return an error if the process is a zombie process.
            print("The process with pid {} ({}) is a zombie process\n".format(str(mainpid), svc))

    return pidchecks

def main():
    cfg = parse_args() # Get arguments for minimal mode and for the configuration file.
    services = [] # Predefine the services list.
    services = load_services(services, cfg) # Get the services into the list by using the cfg file.
    pidlist = get_pid(services) # Get PIDs of the services' processes.
    cpudic = {} # Predefine the dictionary for CPU usage.
    memdic = {} # Predefine the dictionary for RAM usage.
    cpudic, memdic = read_stats(pidlist) # Get stats into the dictionary.
    for (entry, usg) in cpudic.items(): # Print the results.
        print("CPU usage of process {}: {}%".format(entry, usg))
        print("Memory usage of process {}: {}%\n".format(entry, memdic[entry]))

try:
    main() # No need for main module check.
except Exception as err:
    print("A global exception has occured.")
    print(err)
    sys.exit(7)

# print("Time ran: {}".format(datetime.now() - startTime)) # Uncomment if going to benchmark.
\$\endgroup\$
6
\$\begingroup\$

Review

All pieces of code in this answer are untested.

  1. Follow the PEP-8 guidelines. You did this almost flawlessly, but:

    • You missed a couple of variables. Variable names should follow the same naming convention as functions.1

    • One line is 160 characters long. The official line length limit is 79 characters2, but most people enforce a hard limit of 100 characters. 160 columns is too long, though.

  2. The C-era getopt is often more complex to use than argparse, the de facto Python argument parsing library.

  3. When printing error messages to the screen, it's a good idea to write to stderr instead of stdout. In load_services(), for example:

    import sys
    
    ...
    
    print >>sys.stderr, "The file {} most probably does not exist ".format(cfg)
    
  4. Comments describing functions should be docstrings. Docstrings are triple-quoted string literals (multiline string literals) that describe an object. They are placed directly below the object signature, e.g.:

    def foo():
        """I'm a foo() function. Cool, right?"""
    
    
    class Foo:
        """I'm a Foo class. Cool, right?"""
    

    These docstrings don't make any sense, but you get the idea.

  5. If you find yourself explaining the meaning of literals in a comment, they should be constants. Take the exit codes (6 and 7):

    GETOPT_ERROR = 6
    GLOBAL_EXCEPTION_CAUGHT = 7
    

    That's much cleaner. If GETOPT_ERROR is mentioned later in the code, the reader doesn't have to scroll up to understand what it represents. The same goes for the stat file indices:

    PROC_COMM = 1
    PROC_UTIME = 14
    PROC_STIME = 15
    

    If you're not convinced yet, notice how the code does not match the comment:

    utime = int(pidtimes[13]) # utime is the 14th ...
    

    So which of the two is it? Using constants leads to less maintenance errors!

  6. Comments like:

    # Import default system libraries.
    

    ... state the obvious. Simply remove them.

  7. At the very bottom of the file:

    main() # No need for main module check.
    

    That's quite a bold claim. I'd argue name guards are important for every file intended to be used as a script, and there's little reason not to use them.

  8. You only ever use pathlib.Path for pathlib.Path.exists(). You could use os.path.exists() instead.

  9. Avoid catching bare exceptions. Catching Exception will also catch completely unrelated exceptions (KeyboardInterrupt), and exceptions occurring in other parts of the code.

    That being said, the real issue with catching Exception in this case is a lack of separation of concerns. The many layers of if-statements (or function call, see below) already check if the path exists, so why does this:

    except Exception as e: # If such a PID file does not exist, get Main PID from parsing systemctl.
    

    ... suggest that may not be the case? The Exception type encompasses so many other things, heck this doesn't even need to be an OSError instance. That's why it's crucial to factor in the different exceptions a line of code could raise, and write the except block around handling those cases.

  10. If you wish to run a benchmark, time only the relevant code. Taking the initial time at the top of main() works well.

  11. I see no point in making load_services() take a handlerlist parameter. The list will be empty, and if it isn't, a developer can simply call list.extend():

    my_handler_list = ["foo", "bar"]
    my_handler_list.extend(load_services("my_config_file"))
    
  12. Abrvnts ar hrd to rd, y knw? svcservice, usgusage, etc.

  13. These lines:

    cpudic = {} # Predefine the dictionary for CPU usage.
    memdic = {} # Predefine the dictionary for RAM usage.
    cpudic, memdic = read_stats(pidlist) # Get stats into the dictionary.
    

    ... strike me as odd. 'Predefining' doesn't make any sense in Python here, because Python is weakly typed.

  14. In get_pid(), cpuusage is never used beyond assignment.

  15. Good job on using os.path.join(), but you missed a case in get_pid():

    pidfpath = pidfolder + '/' + f # ...
    

    ... should be:

    pidfpath = os.path.join(pidfolder, f) # ...
    
  16. You can easily inline certain sections so you don't have as many variables floating around:

    mchildren = mainproc.children(recursive=True)
    pidchecks.append(mainpid)
    for child in mchildren:
        pidchecks.append(child.pid)
    

    ... could be:

    pidchecks.append(mainpid)
    for child in mainproc.children(recursive=True):
        pidchecks.append(child.pid)
    

    Similarly:

    tmpc = os.listdir(pidfolder)
    for f in tmpc: # ...
    

    ... could be:

    for f in os.listdir(pidfolder): # ...
    

Questions

  1. Is it a good idea to make all those checks for a pidfile in /var/run just to save time from external grepping?

    It depends. Arguments in favor:

    • Other developers don't need to be familiar with the shell commands to understand the code;
    • Implementing something in Python allows you to tune the behavior exactly to your liking;
    • Running pure Python code is normally more secure than issuing shell commands.


    On the other hand:

    • If you don't fully understand what the commands do under the hood, you may make mistakes;
    • It's more work to write and maintain, as you're basically reinventing the wheel;
    • Code you implement yourself is likely less portable than system utilities designed for individual systems.

    I don't have a conclusive answer.

  2. Are all those nested ifs there along with the try / except on top of them a good style, and if not, how would I be able to replace them?

    I'll be frank: they're not good style. How to replace them? Let's take a closer look:

    pidfpath = '/var/run/{}/{}.pid'.format(svc, svc)
    if not Path(pidfpath).exists(): # Most services have a /var/run/service/service.pid file.
        pidfpath = '/var/run/{}.pid'.format(svc)
        if not Path(pidfpath).exists(): # Some services use 'd' after their names for daemon.
            pidfpath = '/var/run/{}.pid'.format(svc + 'd')
            if not Path(pidfpath).exists(): # Others have a /var/run/service.pid file.
                pidfolder = '/var/run/{}'.format(svc)
                tmpc = os.listdir(pidfolder)
                for f in tmpc: # And others have various pidfiles like /var/run/service/pid.
                    f = str(f)
                    if 'pid' in f:
                        pidfpath = pidfolder + '/' + f # Add the file to the dir path. 
    

    My solution would be to extract this snippet into a function, so we can do early returns. Note that this is not the only solution, and there's a heated debate about whether early returns are bad style, but at least this works.

    def get_pidf_path(svc):
        pidfpath = '/var/run/{}/{}.pid'.format(svc, svc)
        if Path(pidfpath).exists():
            return pidfpath
    
        # Most services have a /var/run/service/service.pid file.
        pidfpath = '/var/run/{}.pid'.format(svc)
        if Path(pidfpath).exists():
            return pidfpath
    
        # Some services use 'd' after their names for daemon.
        pidfpath = '/var/run/{}.pid'.format(svc + 'd')
        if Path(pidfpath).exists():
            return pidfpath
    
        # Others have a /var/run/service.pid file.
        pidfolder = '/var/run/{}'.format(svc)
        tmpc = os.listdir(pidfolder)
        for f in tmpc:
            f = str(f)
            # And others have various pidfiles like /var/run/service/pid.
            if 'pid' in f:
                pidfpath = pidfolder + '/' + f
                # Add the file to the dir path.
        return pidfpath
    

    Now capture the function call in a try / except construct:

    try:
        pidfpath = get_pidf_path(svc=svc)
        with open(pidfpath, 'r') as pidf:
            mainpid = int(pidf.readline().strip())
    except Exception as e:
        try:
            mainpid = int(subprocess.check_output("systemctl status {} | grep 'Main PID: ' | grep -Eo '[[:digit:]]*' | head -n 1".format(svc), shell=True))
        except ValueError as e:
            pass
    
  3. Is it a bad practice to manually implement the gathering of information about CPU usage, while still using an external library for memory usage?

    No, that's fine. The points I made as a response to your first question still apply here. If getting CPU data manually becomes a burden, just use a library! They're there to make your life easier.

References

1 PEP-8: Naming Conventions: Prescriptive: Naming Conventions: Function and Variable Names

2 PEP-8: Code lay-out: Maximum Line Length

\$\endgroup\$
  • \$\begingroup\$ Wow, that's good to read. \$\endgroup\$ – qräbnö Jul 20 '18 at 15:51
  • \$\begingroup\$ Still going over some of the topics covered in your answer, I believe I've improved a lot thanks to your review. Really comprehensive, great job! I probably still make some of the same mistakes, but one way to see. \$\endgroup\$ – Fanatique Sep 12 '18 at 8:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.