Linux bonding status check

Question

I have created the below code which works for me, but I feel there might be a better way to accomplish this in a new Pythonic way or better Python syntactic way.

Code snippet:

bondCheck.py

#!/usr/bin/python
# Port bonding checker.
# cat /proc/net/bonding/bond0

import sys
import re

def usage():
        print '''USAGE: %s [options] [bond_interface]

Options:
        --help, -h      This usage document

Arguments:
        bond_interface  The bonding interface to query, eg. 'bond0'. Default is 'bond0'.
''' % (sys.argv[0])
        sys.exit(1)

# Parse arguments
try:
        iface = sys.argv[1]
        if iface in ('--help', '-h'):
                usage()
except IndexError:
        iface = 'bond0'

# Grab the inf0z from /proc
try:
        bond = open('/proc/net/bonding/%s' % iface).read()
except IOError:
        print "ERROR: Invalid interface %s\n" % iface
        usage()

# Parse and output
active = 'NONE'
Link = 'NONE'
slaves = ''
state = 'OK'
links = ''
bond_status = ''
for line in bond.splitlines():
        m = re.match('^Currently Active Slave: (.*)', line)
        if m:
                active = m.groups()[0]

        m = re.match('^Slave Interface: (.*)', line)
        if m:
                s = m.groups()[0]
                slaves += ', %s' % s

        m = re.match('^Link Failure Count: (.*)', line)
        if m:
                l = m.groups()[0]
                links += ', %s' % l

        m = re.match('^MII Status: (.*)', line)
        if m:
                s = m.groups()[0]
                if slaves == '':
                        bond_status = s
                else:
                        slaves += ' %s' % s
                if s != 'up':
                        state = 'FAULT'

print "%s: %s (%s), Active Slave: %s, PriSlave: %s (%s), SecSlave: %s (%s), LinkFailCountOnPriInt: %s, LinkFailCountOnSecInt: %s"  % (iface, state, bond_status, active, slaves.split(',')[1].split()[0], slaves.split(',')[1].split()[1], slaves.split(',')[2].split()[0], slaves.split(',')[2].split()[1], links.split(',')[1], links.split(',')[2])

Code Returned Result:

$ ./bondCheck.py
bond0: OK (up), Active Slave: ens3f0, PriSlave: ens3f0 (up), SecSlave: ens3f1 (up), LinkFailCountOnPriInt:  0, LinkFailCountOnSecInt:  0

Edit:

Bond config

cat /proc/net/bonding/bond0
Bonding Mode: fault-tolerance (active-backup)
Primary Slave: ens3f0 (primary_reselect always)
Currently Active Slave: ens3f0
MII Status: up
MII Polling Interval (ms): 100
Up Delay (ms): 2500
Down Delay (ms): 300
Slave Interface: ens3f0
MII Status: up
Speed: 20000 Mbps
Duplex: full
Link Failure Count: 0
Permanent HW addr: 5a:19:bb:00:00:48
Slave queue ID: 0
Slave Interface: ens3f1
MII Status: up
Speed: 20000 Mbps
Duplex: full
Link Failure Count: 0
Permanent HW addr: 5a:19:bb:00:00:49
Slave queue ID: 0

Answer 1

As a rule I don't like to trigger exceptions, even controlled ones. One reason is that they are more expensive to handle, and as the name implies they should remain exceptions.

So rather than doing this:

try:
        bond = open('/proc/net/bonding/%s' % iface).read()
except IOError:
        print "ERROR: Invalid interface %s\n" % iface
        usage()

I would have something like this:

import sys, os

iface = 'bond0'
bond = ('/proc/net/bonding/%s' % iface)

if not os.path.exists(bond):
    print "ERROR: Invalid interface %s\n" % iface
    sys.exit(1)

# continue

NB: seeing that you are using Python 2.x I have tried to provide compatible code.

Rather than blindly trying to read a file that may not be there in the first place, I simply use os.path to ascertain its existence (could use the isfile function as well).

I also return an exit code, which could be useful if you will be calling this script from another bash script, then you might like to know if the output was as expected or if there is a possible error like a broken network link. Rather than just return 0 (success) or 1 (failure) you could adjust the value depending on the kind of error encountered. Then it's useful to have the special codes in mind.

I see that you are already using sys.exit(1) in your code, which is excellent, but it is in the usage function, where it's the least useful. Consider returning an exit code in the other parts of the code including exceptions. One way of doing it would be to have a main try catch block with a finally so that an exit code is always returned, which could be 0 by default.

You can still have exception handling in this block though, because you might run into some permission issue, depending on the user and perhaps the OS. It's just that I would avoid raising exceptions for situations that are easily anticipated and tested.

---

Consider logging exceptions and even console output or debugging information. A recent discussion on the topic: Custom exception handling function or logging library?

This is especially important for unattended scripts. In this case, it's probably less important if the script will always be run interactively. But it's good to have a trace on file when you are working in some tiny SSH or screen terminal session with a small buffer size and difficulty scrolling text.

---

Regarding semi-constant variables like /proc/net/bonding/ or bond0 I try to regroup them together on top of the script. The paths and interface names may vary from an OS to another and porting code to another platform is easier when essential variable names are not scattered all over the place. Obviously, they should be defined only once and repetition is to be avoided absolutely. I develop scripts for servers that run on at least 3 different flavors of Linux and I sometimes have surprises...

---

Regarding the code proper: the final line doing the print is quite hard to read. All that splitting is unnecessary and even unsafe. If the output of /proc/net/bonding/bond0 changes, or the number of slaves is less than 2, your code could fail.

Ideally, you should have ready-to-use variables and just print them. All the processing, parsing, validation etc has to take place before.

Let's take this snippet as an example:

    m = re.match('^Slave Interface: (.*)', line)
    if m:
            s = m.groups()[0]
            slaves += ', %s' % s

Your code iterates twice. At the first iteration the value of slaves is: , ens3f0. At the second iteration it is: , ens3f0 up, ens3f1. Note the leading comma. It seems that you are not yet familiar with Python structures such as lists or dictionaries, so you are resorting to string manipulations techniques that are superfluous. Here how I would do it, using a simple list:

First add this somewhere in your code, for example before the for loop:

slave_interfaces = []

We simply define an empty list. Then your code becomes:

    m = re.match('^Slave Interface: (.*)', line)
    if m:
            slave_interfaces.append(m.group(1).strip()) 

When a match if found, the value is appended to the list, which means adding an element. Note the addition of strip() to trim whitespace that may surround the interface name.

The regular expression can not only match but capture as well. Since m.group(1) already contains the name of the interface you can use it. Now, at the first iteration the value of slave_interfaces is: ['ens3f0']. At the second iteration it is: ['ens3f0', 'ens3f1']. You have a list of two elements, and you can address each of them by index number, thus slave_interfaces[0] = 'ens3f0' and slave_interfaces[1] = 'ens3f1'. You can check the number of elements: len(slave_interfaces) will return 2. So you know in advance that using an index number greater then 1 will result in an error (IndexError: list index out of range).

Producing a comma-separated list is as easy as joining:

','.join(slave_interfaces)

=> 'ens3f0,ens3f1'

---

On a final note, for parsing arguments and if you have Python 2.7 at least, the argparse library provides more flexibility. Have a look at the add_help function too. When you develop more sophisticated scripts with multiple arguments that may be in no particular order, your way of doing it will not scale well.