Page MenuHomeVyOS Platform

Requirements for partial interface setup
Open, Requires assessmentPublic

Description

It was recently agreed that on configuration, only interface changes should be applied (vs reapplying the whole interface configuration). This change should be quite straight forward (there may be a number of option dependencies when one value change requires other to be re-applied) but one of the must-have requirement is that code must now track the keys which have been set in the configuration.

Looking at how it could be implemented, I noticed that the interface configuration code has been changed to use jmespath.search

The api of the call is indeed more pleasant that using raw python dict:

tmp = jmespath.search('ipv6.address.no_default_link_local', config)

vs

tmp = config.get('ipv6', {}).get('address', {}).get('no_default_link_local', None)

I think this "." notation for the key is nice and could be adopted. This also goes toward showing that some helper code is required to improve configuration handling and make the code of the conf mode extension clearer and easier.

Reading the jmespath code, it is not really optimised for speed but for flexibility. Also, very few features are used, only the search function. The same result could be achieved with a dedicated helper function performing this dict walk instead

Something like (code not tested, here for demonstration)

def search (config, path):
    parts = path.split('.')
    inside = parts[:-1]
    if not inside:
        return config[path]
    c = config
    for p in parts[:-1]:
        c = c.get(p, {})
    return c.get(parts[-1], None)

As we have a need to track which keys are changed, this function could be converted to be a method of ConfModeDict (a subclass of dict) and renamed get.
This would not break/change any of the current code using the dict without the nested search feature.

This would also have the side effect to allow to better typo detection. Should a typo then occur on a key, this could be better caught at runtime and reported with an exception.
Currently, as the dict already contains the default value, should a typo occur, the default value is returned instead.

Subclassing dict would also allow intercepting __setitem__ (the under-the-hood function assigning a value to a dict when [] = is used). __delitem__ would also need to be redefined (as it would mean that any set value was unset). It would then be possible to know if a value was set or not and get a list of only the values which were set.

The class could also contain the "defaults" inside, allowing to compare any set value with its default and not returning it, if the value explicitly set is still the default.

The API I would suggest for this class would be something like this (suggestion only until real implementation helps with details):

class ConfModeDict(dict):
    def __init__(self, defaults):
         self._configured = dict([(k,False) for k in defaults])
         self._defaults = deepcopy(defaults)
    def get(self, key, default=False):
    def configured(self, include_defaults=False): # return a list keys which were configured, if True include values even if what was configured is the default

    # to allow the interception when data is set in the dict
    def __setitems__(self, k, v):
    def __delitems__(self, k):

if implementated for the interface code, this change new class could easily be used in place of the current dict. Only the setup of the dict would have to change, using it more widely should not break any of the conf modules.

The documentation would "only" be updated to let users know that ConfModeDict should be used when writing conf code, but this transition can be progressive.

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

thomas-mangin created this object in space S1 VyOS Public.
thomas-mangin updated the task description. (Show Details)Aug 2 2020, 9:41 AM
c-po added a comment.Aug 2 2020, 12:30 PM

I like the idea of adding our own implementation which mimics jmespath.search() so this definately a +1.

About the idea of ConfModeDict I personally find it too complex. When looking at get_interface_dict() a lot of plumbing is done inside and it uses common functions like leaf_node_changed() to determine if a certain path was altered or not. I would more prefer a simple function name get_changed_keys() which will just return a dict with the changed keys and its content. The idea might be more or less identical in the backend but I feel a simpler design will get easier adopted..

thomas-mangin added a comment.EditedAug 3 2020, 10:48 PM

Thank you for the feedback, even if it was not what I was hoping for !

It may look complicated, the code doing the work would be way shorter than the post above describing the concept.

Something like get_changed_keys() is how my thinking started and after some thinking the idea evolved until I reached the conclusion that this class approach was the simplest way for this fearure. The dict changes are really not complicated once understood and would be easy to use.

If anyone want to show me wrong, I will happily let someone else implement this. If this idea is not the right way, then I will have to ask someone else to pick T2738 and implement the differential interface apply, so I can see what would be a "better" solution.

Still I believe this is the right path and that if enough time is spent on the implementation, this is how the code will end up looking. In a similar way, there was natural resistance when I tried to discuss moving more of the network code in the Interface class. As commit are showing, I stopped working on that code as I could not get the team to agree but it was now done as with time it became clear that it was the right way forward.

pasik added a subscriber: pasik.Aug 24 2020, 1:00 PM