Page MenuHomeVyOS Platform

"set interfaces" Python handler code improvements - next iteration
Needs testing, NormalPublicFEATURE REQUEST

Description

Moving the INterface configuration code from VyOS 1.2 to 1.3 it already passed two or three (depending on how you count) iteration cycles. In the beginning I did a PoC using python-iproute2 which very fast showed its limited use in VyOS thus a "general" abstraction was created called "vyos.ifconfig". This was again refactored and restructured by @thomas-mangin to reduce duplicated code and make it more reusable.

This changed was only a change from the inside of this library and the calling code was almost of no change. By adding new functionalities like VRF the design again showed its still not ideal and as I wanted it to be in the end. More and more errors arised by copy/apsting code between classes to configure all kind of interface stuff.

Current Design

Some but not all poor design decisions (I guess I have introduced them all?)

  • Every class (interface-{ethernet, dummy, bridge..}) calls Interface().set_alias() or add_addr()
  • Every class needs to manually escape sequences "what can be changed when interface is online, what can only be done if interface is admin down?"

The former beeing one of the worst designs - duplicated code. The following picture shows who are the offenders calling Interface() class, every caller duplicates most of the code

New Design

The general Interface() class is still used with all its internal functions but while moving towards get_config_dict() based implementation a summerisation function Interface().update() is added which takes a config dict as its argument.
Thus general settings which are done for each and every interface can be rewritten at a single location and reused from now on.

Other "special" implementations required for e.g. bond or bridge interfaces, should happen in its derived class overwriting the update() function. Thus adding new features will become much easier as it only needs to be added in very few places.

One could also think of a helper class called by verify() which does some basic checking (VRF for instance) further reducing the amount of duplicated code.

Rules

As this is a possible candidate to really fuck up things, for each change there must be a proper test available in vyos-smoketest first so we stop "bad" ISO images from getting out into the wild.

Details

Difficulty level
Hard (possibly days)
Version
-
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Behavior change

Event Timeline

c-po changed the task status from Open to In progress.Jun 26 2020, 1:50 PM
c-po triaged this task as Low priority.
c-po created this task.
c-po claimed this task.Jun 26 2020, 4:36 PM
c-po raised the priority of this task from Low to Normal.
c-po changed Difficulty level from Unknown (require assessment) to Hard (possibly days).
c-po changed Is it a breaking change? from Unspecified (possibly destroys the router) to Behavior change.
pasik added a subscriber: pasik.Jun 28 2020, 7:07 PM
thomas-mangin added a comment.EditedJun 29 2020, 9:31 AM

Agreeing and defining the name of the keys to be used in the dict passed to verify/generate/apply, would be very beneficial for the project.
The current code does this by calling a number of helper function (so that all interface have the same keys) but this is not defined or/and enforced.

The format of the data is too "loose" and therefore it is not possible to perform good validation, such as using a FixedDict (which would only allow good known keys).

The Interface code was refactored so that most call to setup things can be done via set_interface (each individual function now calling it). It is underpinned by the data in Interface such as _command_{sg}et, _sysfs_{sg}et.

It would make sense to have the dict in the configuration code use these keys ('alias', 'mac', 'mtu, 'arp_filter', etc.) when configuring data. It would allow iterating over all the keys and if the key exists, it will be used to configure the interface.

A full list can be provided by a helper function. The tunnel code does something similar (but does not go as far as this).

This comment was removed by elianora.
This comment was removed by dsummers.
runar added a subscriber: runar.Jul 2 2020, 4:42 PM

Hi @elianora,

Please open a new ticket or move your comment to an appropriate ticket, this ticket is not discussing your consernes.

elianora added a comment.EditedJul 2 2020, 6:53 PM

EDIT 1: Removing comment after speaking with cpo, I apologize for the confusion

EDIT 2: also, @dsummers, if you could please message me on Slack - I do not see a way to message directly on the site and cannot find you in the slack and would like to ask questions without adding any more unnecessary info on this ticket.

@elianora Email me at david@summersoft.fay-ar.us

c-po changed the task status from In progress to Needs testing.Jul 25 2020, 3:53 PM
c-po changed the status of subtask T2733: Support MTU configuration on pseudo ethernet devices from Open to In progress.Jul 26 2020, 7:44 AM
c-po changed the status of subtask T2734: WireGuard: fwmark CLI definition is inconsistent from Open to Backport candidate.Jul 26 2020, 11:17 AM
c-po changed the status of subtask T2746: IPv6 link-local addresses not configured from Open to In progress.Jul 30 2020, 9:12 PM