Page MenuHomeVyOS Platform

Ensure configration mode scripts conform to coding guidelines
Open, Requires assessmentPublic

Description

There are several config_mode scripts (11) (6) that break compatibility with the script structure and behaviour as outlined in:

https://docs.vyos.io/en/latest/contributing/development.html#configuration-script-structure-and-behaviour

The non-standard use of vyos.config Config in those scripts can likely be avoided by the move to get_config_dict as in the example here [SSH: migrate to get_config_dict()]:

https://phabricator.vyos.net/T2635

There are compelling reasons to follow the guidelines outlined above:

  1. Ability to mock Config() with test data and run scripts in batch for test based development
  2. Ability to run all scripts from a config daemon
  3. Easy transition to vyconf

Some of the non-conformant scripts are easily adjusted without changing logic or readability; others rely on the use of Config() in 'verify' in a manner that should be considered as a candidate for a get_config_dict rewrite.

This task will serve as a metatask for investigation; the list of non-conforming scripts is below:

arp.py
dns_forwarding.py
flow_accounting_conf.py
host_name.py
interfaces-bonding.py
interfaces-bridge.py
interfaces-tunnel.py
interfaces-wireless.py
protocols_bfd.py
snmp.py
system-proxy.py

Details

Difficulty level
Unknown (require assessment)
Version
vyos-1.3
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)

Event Timeline

jestabro created this object in space S1 VyOS Public.

@jestabro could you please clarify how interfaces-tunnel.py is not following the guideline. The class it uses to generate the dict is internal to the get_config() function and the dict API is respected.

Only that it inherits from Config, hence the form of the Config instance can not be changed, say, to 'off-line' Config, or 'testing' Config, which would have different init functions; if it took Config as an argument instead, or a named argument with default the 'live' Config, then it would not be an issue for generalization. But we should discuss the details further, and collect results and suggestions.

There is really no difference between calling Config() and using it functions in the code - as is done by every module - vs having the class inherit from it as I do not use any private ( _ prefixed ) functions. I can modify the code to have self.config as an object of the class instead but IMO this is cosmetic and does not change anything with the API and coding guideline.

I have no objection to adding a new rule asking for no subclassing Config (or any part of the rest of the VyOS codebase) in modules but in that case, we should update the coding guideline to say what can and can not be done.

I'm not sure that we have to go that far, but let me take a closer look, and we can discuss; the model is to be able to call, in batch, the functions of an arbitrary conf_mode script, without variation in structure or behaviour.

jjakob added a subscriber: jjakob.Jun 25 2020, 1:56 PM

Keep in mind that the preferred way to implement scripts is, in my mind, the one used by interfaces-tunnel.py: it takes both session and effective configs and compares them, only applying the changes for the differences that are required. get_config_dict just takes the session config and so it requires a complete teardown and re-initialization of any system component that is configured from it (restarting instead of reloading services, deleting interfaces instead of modifying just 1 setting, ...)
I don't consider just get_config_dict as the preferred future way to implement features for that reason, rather it is the interfaces-tunnel that could be made the reference.
get_config_dict could be ran 2 times (once with effective=True) in each script, then the script could compare/make a diff of the 2 dicts, but that's what interfaces-tunnel ConfigurationState does.
Take for example a minor change in the current openvpn code - changing the description of the interface - currently results in a complete service outage (restart). AFAIK all scripts are like this. It didn't use to be that way in Vyatta, most perl scripts compared session and effective configs and just applied the necessary changes.

I agree @jjakob that we are still converging on the best way to structure the salient information, namely the difference between effective and session configs, however this will be internalized: a simple example of use for testing and daemon (and other) is:

https://github.com/vyos/vyos-1x/compare/current...jestabro:vyos-configd#diff-ad54a873d8a788700ff3cfa1b18c615dR71

To be fair, I'm getting frustrated by the layers-upon-layers of new abstractions getting added on old code that doesn't work properly in the first place. I'd much prefer if we just started with a clean slate. I liked @thomas-mangin 's idea of replacing vyatta-cfg completely with our own code, either vyconf or his python daemon. I wouldn't waste time patching up the old backend, just make a decision in one place to replace it completely.

@jjakob that is in fact what we are doing: for stability, some of the changes are necessarily incremental --- these are not new abstractions, but rather, refinements of the existing ones, which allow the separation of new code from legacy. We are avoiding any patching of the old backend, simply replacing with the structures which will allow a switch to vyconf, while improving performance and extension of the new code.

pasik added a subscriber: pasik.Jun 25 2020, 8:36 PM
jestabro updated the task description. (Show Details)Aug 11 2020, 1:15 AM
jestabro updated the task description. (Show Details)
jestabro updated the task description. (Show Details)Aug 14 2020, 5:03 PM