Page MenuHomeVyOS Platform

"set interfaces" Python handler code improvements - next iteration
Closed, ResolvedPublicFEATURE 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

image.png (921×1 px, 182 KB)

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

Related Objects

StatusSubtypeAssignedTask
In progressFEATURE REQUESTNone
ResolvedFEATURE REQUESTNone
ResolvedFEATURE REQUESTc-po
Resolvedc-po
ResolvedBUGc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedBUGc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedBUGc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedENHANCEMENTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
ResolvedFEATURE REQUESTc-po
Resolvedc-po
ResolvedFEATURE REQUESTc-po

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
c-po changed the status of subtask T3043: Wireless: Refactor CLI from Open to In progress.Nov 3 2020, 4:38 PM
erkin set Issue type to Internal change (not visible to end users).Aug 29 2021, 2:02 PM
erkin removed a subscriber: Active contributors.