Page MenuHomeVyOS Platform

Ensure configration mode scripts conform to coding guidelines
Closed, ResolvedPublic

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)
Issue type
Internal change (not visible to end users)

Event Timeline

@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.

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.

jestabro updated the task description. (Show Details)
erkin set Issue type to Internal change (not visible to end users).Aug 29 2021, 2:05 PM
erkin removed a subscriber: Active contributors.

This was a meta-task for conformance to coding guidelines for inclusion of scripts in vyos-configd. Those guidelines are documented and checked in the smoketest test_configd_inspect.py for both Sagitta and Equuleus, and will be closed.

jestabro edited projects, added VyOS 1.3 Equuleus; removed VyOS 1.3 Equuleus (1.3.3).
jestabro moved this task from Need Triage to Finished on the VyOS 1.3 Equuleus board.