Difference between revisions of "Python coding guidelines"
(Created page with "= Preamble = The switch to the Python programming language for new code is not merely a change of the language, but a chance to rethink and improve the programming approach....")
|Line 1:||Line 1:|
= Preamble =
= Preamble =
Revision as of 17:00, 4 July 2019
The switch to the Python programming language for new code is not merely a change of the language, but a chance to rethink and improve the programming approach.
Let's face it: VyOS is full of spaghetti code where logic for reading the VyOS config, generating daemon configs, and restarting processes is all mixed up. Python (or any other language, for that matter) does not provide automatic protection from bad design, so we need to also devise design guidelines and follow them to keep the system extensible and maintainable.
Configuration script structure and behaviour
Configuration scripts **must** follow the following structure:
import sys from vyos.config import Config from vyos.util import ConfigError def get_config(): vc = Config() # Convert the VyOS config to an abstract internal representation config = ... return config def verify(config): # Verify that configuration is valid if invalid: raise ConfigError("Descriptive message") return True def generate(config): # Generate daemon configs pass def apply(config): # Apply the generated configs to the live system pass try: config = get_config() verify(config) except ConfigError as e: print(e) sys.exit(1)
The get_config() function must convert the VyOS config to an abstract internal representation. No other function is allowed to call vyos.config.Config object methods directly. The rationale for it is that when config reads are mixed with other logic, it's very hard to change the config syntax since you need to weed out every occurence of the old syntax. If syntax-specific code is confined to a single function, the rest of the code can be left untouched as long as the internal representation remains compatible.
Another advantage is testability of the code. Mocking the entire config subsystem is hard, while constructing an internal representation by hand is way simpler.
The verify() function takes an internal representation of the config and checks if it's valid, otherwise it must raise VyOSError with an error message that describes the problem and possibly suggests how to fix it. It must not make any changes to the system. The rationale for it is again testability and, in the future when the config backend is ready and every script is rewritten in this fashion, ability to execute commit dry run ("commit test" like in JunOS) and abort commit before making any changes to the system if an error is found in any component.
The generate() function generates config files for system components.
The apply() function applies the generated configuration to the live system. It should use non-disruptive reload whenever possible. It may execute disruptive operations such as daemon process restart if a particular component does not support non-disruptive reload, or when the expected service degradation is minimal (for example, in case of auxillary services such as LLDPd). In case of high impact services such as VPN daemon and routing protocols, when non-disruptive reload is supported for some but not all types of configuration changes, scripts authors should make effort to determine if a configuration change can be done in a non-disruptive way and only resort to disruptive restart if it cannot be avoided.
Unless absolutely necessary, configuration scripts should not modify the active configuration of system components directly. Whenever at all possible, scripts should generate a configuration file or files that can be applied with a single command such as reloading a service through systemd/SysV init. Inserting statements one by one is particularly discouraged, for example, when configuring netfilter rules, saving them to a file and loading it with iptables-restore should always be preferred to executing iptables directly.
The apply() and generate() functions may raise ConfigError if, for example, the daemon failed to start with the updated config. It shouldn't be a substitute for proper config checking in the verify() function. All reasonable effort should be made to verify that generated configuration is valid and will be accepted by the daemon, including, when necessary, cross-checks with other VyOS configuration subtrees.
Exceptions, including VyOSError (which is raised by vyos.config.Config on improper config operations, such as trying to use list_nodes() on a non-tag node) should not be silenced or caught and re-raised as config error. Sure this will not look pretty on user's screen, but it will make way better bug reports, and help users (and most VyOS users are IT professionals) do their own debugging as well.
Python 3 shall be used. How long can we keep Python 2 alive anyway?
No considerations for Python 2 compatibility should be taken.
Tabs shall not be used. Every indentation level should be 4 spaces.
Template processor should be used for generating config files. Built-in string formatting may be used for simple line-oriented formats where every line is self-contained, such as iptables rules. Template processor must be used for structured, multi-line formats such as those used by ISC DHCPd.
The default template processor for VyOS code is jinja2.