4
\$\begingroup\$

I've written my first "real" Python application. I've never worked much with Python before, so I'd like to receive feedback on how I can structure the application to make it follow the way Python programs usually are.

This is very much from a "readability" perspective, but getting feedback on general structure as well as my use of classes, naming of methods and use of comments would be great to get other's opinion on.

https://github.com/sbrattla/swarmconstraint

The use case for this application is real. I manage a Docker Swarm, but I need a service (aka task in Docker Swarm) to only run in a couple of nodes participating in that swarm. However, if I use placement constraints (constraint a service to specific nodes) and the nodes which the service is constrained to go down - then the service goes down as well. So, the application will remove the placement constraints if specified nodes goes down so that the service can "fallback" to other nodes.

#!/usr/bin/python3

import argparse
import docker
import json
import logging
import re
import string
import time

class SwarmConstraint:

  def __init__(self, args):
    self.args = args
    self.initClient()

    self.logger = logging.getLogger(__name__)
    handler = logging.StreamHandler()
    formatter = logging.Formatter(
        '%(asctime)-25s  %(levelname)-8s  %(message)s')
    handler.setFormatter(formatter)
    self.logger.addHandler(handler)
    self.logger.setLevel(logging.DEBUG)

    if (not self.args['watch']):
      raise Exception('At least one node to watch must be provided.')

    if (not self.args['toggle']):
      raise Exception('At least one node to toggle must be provided.')

    if (not self.args['label']):
      raise Exception('At least one label must be provided.')

    if (not self.args['prefix']):
      raise Exception('A prefix must be provided.')

    self.logger.info('Watch {watch}.'.format(watch=','.join(self.args['watch'])))
    self.logger.info('Toggle the label(s) {labels} on {toggle}.'.format(labels=','.join(self.args['label']), toggle=','.join(self.args['toggle'])))
    self.logger.info('Prefix disabled labels with {prefix}.'.format(prefix=self.args['prefix']))

  def run(self):

    # Collect availability for watched nodes, and keep track of the collective
    # availability for all the watched nodes.
    nodes = self.getNodes()
    allWatchedNodesUnavailable = True
    for nodeId in nodes:
      watchNode = nodes[nodeId]
      if (not self.args['watch'] or watchNode['hostname'] not in self.args['watch']):
        continue

      if (self.isNodeAvailable(watchNode) == True):
        allWatchedNodesUnavailable = False
        break;

    if (allWatchedNodesUnavailable):
      self.logger.warn('All watched nodes are unavailable.')
    else:
      self.logger.debug('One or more watched nodes are available.')

    # Disable or enable labels depending on the collective availability for all
    # the watched nodes. 
    for nodeId in nodes:
      toggleNode = nodes[nodeId]
      if (self.args['toggle'] and toggleNode['hostname'] not in self.args['toggle']):
        continue

      if (allWatchedNodesUnavailable):
        self.disableLabels(toggleNode, self.args['label'], self.args['prefix'])
      else:
        self.enableLabels(toggleNode, self.args['label'], self.args['prefix'])

  def getSocket(self):
    return 'unix://var/run/docker.sock'

  def initClient(self):
    # Initialize the docker client.
    socket = self.getSocket()
    self.client = docker.DockerClient(base_url=socket)

  def getNodes(self):
    # Returns all nodes.
    allNodes = self.client.nodes.list();
    allNodesMap = {}
    for node in allNodes:
      allNodesMap[node.id] = {
        'id' : node.id,
        'available' :  True if node.attrs['Spec']['Availability'] == 'active' else False,
        'hostname': node.attrs['Description']['Hostname'],
        'role' : node.attrs['Spec']['Role'],
        'platform' : {
          'os' : node.attrs['Description']['Platform']['OS'],
          'arch' : node.attrs['Description']['Platform']['Architecture']
        },
        'labels' : node.attrs['Spec']['Labels'],
      }

    return allNodesMap

  def isNodeAvailable(self, node):
    return node['available']

  def disableLabels(self, node, labels, prefix):
    # Disable labels on a node by adding a prefix to each label. The node will only be
    # updated if at least one of the provided labels are currently enabled.
    matchingNode = next(iter(self.client.nodes.list(filters={'id':node['id']})), None)
    if (matchingNode is None):
      return

    spec = matchingNode.attrs['Spec']
    update = False

    for label in labels:
      if (label not in spec['Labels']):
        continue

      nodeLabelKey = label
      nodeLabelVal = spec['Labels'][nodeLabelKey]
      spec['Labels'].update(self.prefixNodeLabel(nodeLabelKey, nodeLabelVal, prefix))
      spec['Labels'].pop(nodeLabelKey, None)
      update = True

      self.logger.info('Disabling the label "{key}={val} on {node}".'.format(key=nodeLabelKey, val=nodeLabelVal, node=node['id']))

    if (update):
      matchingNode.update(spec)
      return True
    else:
      return False

  def enableLabels(self, node, labels, prefix):
    # Enable labels on a node by removing the prefix from each label. The node will only be 
    # updated if at least one of the provided labels are currently disabled.
    matchingNode = next(iter(self.client.nodes.list(filters={'id':node['id']})), None)
    if (matchingNode is None):
      return

    spec = matchingNode.attrs['Spec']
    update = False

    for label in labels:
      label = self.prefixLabel(label, prefix)
      if (label not in spec['Labels']):
        continue

      nodeLabelKey = label
      nodeLabelVal = spec['Labels'][nodeLabelKey]
      spec['Labels'].update(self.unPrefixNodeLabel(nodeLabelKey, nodeLabelVal, prefix))
      spec['Labels'].pop(nodeLabelKey, None)
      update = True

      self.logger.info('Enabling the label "{key}={val} on {node}".'.format(key=nodeLabelKey, val=nodeLabelVal, node=node['id']))

    if (update):
      matchingNode.update(spec)
      return True
    else:
      return False

  def prefixLabel(self, label, prefix):
    # Split and prefix a label into a dictionary holding the prefixed key and the value separately.
    return '{prefix}.{key}'.format(prefix=prefix, key=label)

  def isNodeLabelPrefixed(self, key, prefix):
    # Evaluates if a node label is prefixed
    return True if key.find(prefix) > -1 else False;

  def prefixNodeLabel(self, key, val, prefix):
    # Prefix a node label.
    label = {'{prefix}.{key}'.format(prefix=prefix,key=key) : '{val}'.format(val=val)}
    return label

  def unPrefixNodeLabel(self, key, val, prefix):
    # Remove prefix from a node label.
    key = key.replace('{prefix}.'.format(prefix=prefix), '')
    label = {'{key}'.format(prefix=prefix,key=key) : '{val}'.format(val=val)}
    return label

class FromFileAction(argparse.Action):

  def __init__(self, option_strings, dest, nargs=None, **kwargs):
    super(FromFileAction, self).__init__(option_strings, dest, **kwargs)

  def __call__(self, parser, namespace, path, option_string=None):
    if (path):
      data = None
      with open(path) as f:
        data = json.load(f)

      if (data is None):
        return

      if ('watch' in data):
        namespace.watch += data['watch']

      if (data['toggle']):
        namespace.toggle += data['toggle']

      if ('label' in data):
        namespace.label += data['label']

    return

def main():
  parser = argparse.ArgumentParser(description='Toggles one or more constraints depending on node availability')
  parser.add_argument('--watch', metavar='watch', action='append', default=[], help='A node which availability is to be watched.')
  parser.add_argument('--toggle', metavar='toggle', action='append', default=[], help='A node for which constraints are to be toggled. Defaults to all nodes.')
  parser.add_argument('--label', metavar='label', action='append', default=[], help='A label which is to be toggled according to availability for watched nodes.')
  parser.add_argument('--prefix', metavar='prefix', default='disabled',  help='The prefix to use for disabled labels. Defaults to "disabled".')
  parser.add_argument('fromFile', action=FromFileAction, help='A file which holds configurations.')

  args = vars(parser.parse_args())
  se = SwarmConstraint(args)

  while(True):
    try:
      se.run()
      time.sleep(10)
    except KeyboardInterrupt:
      break
    except Exception as err:
      print(err)
      break

if __name__ == '__main__':
  main()
\$\endgroup\$
3
  • \$\begingroup\$ Thanks @BCdotWEB, updated accordingly! I hope this makes it a bit easier for people to understand what I'd like feedback on. \$\endgroup\$ Commented Oct 14, 2020 at 19:27
  • 1
    \$\begingroup\$ @Emma is that a built-in functionality of ZooKeeper? \$\endgroup\$ Commented Oct 14, 2020 at 20:11
  • 1
    \$\begingroup\$ @Emma so, what the application here does is that it turns Docker Swarm's placement constraints (hard constraints) into soft constraints. You mention both ZooKeeper, Terraform and Ansible in your comment. Are we on the same page? The tools you mention, such as ZooKeeper, is quite a handful in itself to run. Would you suggest setting up a cluster of ZooKeeper instances just to implement this functionality? Would you use Ansible to watch availability of individual services, and similarly use Ansible to react to availability changes? I use SaltStack for provisioning. Not sure I'd use it for this? \$\endgroup\$ Commented Oct 14, 2020 at 21:02

1 Answer 1

Reset to default
4
\$\begingroup\$

Welcome to Code Review!

PEP-8

In python, it is common (and recommended) to follow the PEP-8 style guide for writing clean, maintainable and consistent code.

Functions and variables should be named in a lower_snake_case, classes as UpperCamelCase, and constants as UPPER_SNAKE_CASE.

You do not need to specify parentheses around the conditional checks in if-/elif- clauses.

Indentation for the program should follow a 4-whitespace for each indent level style.

f-strings over str.format

Of the 2 of following, which would you prefer? (both do the same thing)

  • f"{var} value"
  • "{var} value".format(var=var)

The former is called f-string, and is newly introduced in python 3.

argparse

You are gathering all arguments from the command line using argparse package, yet instead of letting argparse set those as required, you are validating in the class initialisation.

Magic string

The unix socket address appears to be a constant, and doesn't really need to be associated with the class method. Unless you plan for this to be also provided from the cli, let it be defined as a constant value. Same goes for logging formatter etc.

Useless complexity

You gather all nodes from the docker swarm client into a variable, whose only purpose is to get iterated upon, with no further reference to it. Iterate without storing a separate variable.

Generating the node object's dictionary can be separated into its own method, which received a node and returns the dictionary you need.

The node aggregation is defined as get_nodes (the correct naming convention), and still for the disable_labels/enable_labels etc, you aggregate nodes with its own node aggregator.

As an example, the node aggregator can be done as:

@staticmethod
def process_node(node):
    spec = node.attrs["Spec"]
    description = node.attrs["Description"]
    return {
        "id": node.id,
        "available": spec["Availability"] == "active",
        "hostname": description["Hostname"],
        "role": spec["Role"],
        "platform": {
            "os": description["Platform"]["OS"],
            "arch": description["Platform"]["Architecture"],
        },
        "labels": spec["Labels"],
    }

def get_nodes(self):
    return {
        node.id: self.process_node(node)
        for node in self.client.nodes.list()
    }

Notice in the above the node["available"] attribute. You don't need the if-else statement there.

Comments

The comments as they are, are completely useless. If you have something like:

def init_client(self):
    # Initialize the docker client.

then the comment serves no real purpose, as init_client is self-explanatory name. If you do want to add comments, perhaps specify how the function does things (though the code should be doing that).

Duplicated code

Both the functions enable_labels and disable_labels have a majority of the code common in them. This should be extracted to its own function.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.