Page MenuHomeVyOS Platform

Unify creation and maniulation of interfaces
Open, Requires assessmentPublicFEATURE REQUEST

Description

Back on the days where ifconfig.py was a PoC how a new system for managing interfaces could be it now has eveloped to a state where its very modular, but consists out of two brains. Actually those brains are 1 + <numer of interfaces>.

The implementation of vyos.ifconfig should be altered in a way to be more object oriented. The following items should be changed:

  • Every interface-{bond|bridge|ethernet|...}.py file consists out of
    • one big default_config_data dictionary which is partely the same on every interface
    • one step where the dictionary is actually written into the underlaying hardware (partly this is also the same on all interfaces, set_admin_state(), set_mac(), ....

The change should rather provide an extended dictionary from vyos.ifconfig class. The dictionary grows by its inheritance level, every level adds some more entries to the dictionary itself, and in the end interface-{bond|bridge|ethernet|...}.py get_config() will just call config = deepcopy(InterfaceClass(().get_dict()) and the CLI config can now be read into the dictinary which is fully understood by vyos.ifconfig.

Once the dictionary is populated and verified it cna be applied in one single call to vyos.ifconfig.InterfaceClass() and the dictionary is again passed down the inheritancy tree and all settings are applied as single source. Adding / reworking and maintaining the code will thus become much more easy. Also delta checks (some parts can only be changed when interface is administratively down, e.g. MAC address) can be unified in one place.

So from a MAC address example, I need to disable the interface, I do this by calling set_admin_state('down'), the function internally keeps a reference counter and the interface is only disabled once ehwn the counter transits from 0 -> 1, and it is only enabled back when the counter resets itself back to 0 - thus no up/down flaps will occur.

Another good example for this is adding and removing members of a bridge / bond interface.

Details

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

Event Timeline

c-po created this task.Mar 28 2020, 10:24 AM

There this three types of functions which as class can have:

  • "normal" when the first argument is "self"
  • classmethod (using the @classmethod decorator before the function). In that case self replaced from an instance of the class by a reference to the class itself (often called cls, in that case InterfaceClass)
  • staticmethod (where the function does not need class data and is jus placed under the class) can be called with InterfaceClass.func()

get_dict() should be a classmethod.

Also get_dict should take care of the deepcopy and should be config = InterfaceClass.get_dict()

thomas-mangin added a comment.EditedMar 28 2020, 10:52 AM

The recent change in implementation have changed the code from "if/else" to data-driven.
For example, every class now has a "definition" dictionary which indicates what the interface can/cannot do, for example, be bonded or not or it it supports vlan.

It will allow to change code like in list_interface.py from:

elif args.broadcast:
    eth = Interface.listing("ethernet")
    bridge = Interface.listing("bridge")
    bond = Interface.listing("bonding")
    interfaces = eth + bridge + bond

where listing is already a class method of Interface returning the name of the interface under that the named section.
(off-topic, the name should be surely changed from Interface.listing to Interface.section for clarity)

It should lead to something like a "get_capability" classmethod on the Interface class returning for all the classes which are matching a particular attribute.

Then it can lead to something like (not implemented - example)

interfaces = []
for name Interface.get_capablity("broadcast"):  # returns the name/type of the interfaces which can bond (ethernet, dummy, ...)
    interfaces.append(Interface.interfaces(name))  # returns the name of the router interfaces of that type (eth0, dum1)

with no risk of new interface being added without the list_interface missing them.

This is made possible by having all the sub-classes of Interface to "register" (see @Interface.register decorator at the top of the class)

When a dictionary is defined with a parent class and not re-defined by the children, the dict is in effect the same (vs a dict created in init where the value is per instance)

class Parent:
  shared = {}

class Child(Parent):
   pass

Parent and Child then share the "shared" a dictionary.

from vyos.ifconfig import *
EthernetIf._prefixes is DummyIf._prefixes
True

Regarding the reference counter for changes. It can also be implemented by storing in an Interface specific class level dictionary the last know state of the interface.
However, should multiple instances of the class be run by multiple programs then this could become problematic and this limitation should be kept in mind.

There is an issue with storing the state of the interface and then applying it in bulk. For some interface we want to admin it down before performing change and then bringing it back up.
If we store this then only the change and the up command will be applied, this is not going to work.

We could however load the interface state for the field we can get from operational command and then only run then if they do not perform a NOP.
It will remove a few system calls to the OS ( ip link ..), which probably have no effect anyway.

The current behaviour whilst "heavier" has the benefit of getting the interface fully re-setup which is a good thing and could prevent hard to find bugs.

with T2238 interface can be listed using their feature definition (broadcast, bonding, bridge ...).

pasik added a subscriber: pasik.May 9 2020, 8:58 AM