[RFC] Backend minor rework and improvement

[cminyard]

I've been studying the backend and I found a few things.

The first two patches fix a bug and remove unnecessary code.

The next patch applies the DRY principle and consolidates some code.

The last two patches give an alternate interface to clixon_pseudo_plugin() that results in less code being used to call it, and replaces all the calls. It also lets you add plugins from a loadable module and automatically cleans up any added plugins on a failure.

I've been looking at this because I have been looking at using clixon. One thing I would like is the ability to have per-namespace backend plugins. I think this makes sense (though I'm not 100% sure); this way we could have individual plugins for each namespace without each plugin having to scan the entire XML tree for what they need.

It's also not completely clear to me how to use the add/remove/change part of a transaction. You are only getting a little bit of data. You can search up the parents to find where you are in the tree, but it seems inefficient to have to do this on every plugin. But that seems to be the only way you could use this. Again, being able to do this on a per-namespace basis would seem more efficient.

[olofhagsand]

I've been looking at this because I have been looking at using clixon. One thing I would like is the ability to have per-namespace backend plugins. I think this makes sense (though I'm not 100% sure); this way we could have individual plugins for each namespace without each plugin having to scan the entire XML tree for what they need.

Initially we tried many methods, some with higher abstractions, but settled with the current API on a low abstraction level with few assumptions on how the programmer organizes the plugins vs YANG modules. It could be nice to have a more abstract interface based on modules/namespaces which is more coupled to the underlying YANG modules, but I would leave the current API in place for users that have other perspectives. As an example of other perspectives, you could see a plugin based on a target system capability, (some OS configuration) that takes info from multiple YANG modules (some interface info, some DNS, maybe some ACL), and thus needs to collect info from multiple modules. Historically, this is what lead us to the current (low-level) API.

It's also not completely clear to me how to use the add/remove/change part of a transaction. You are only getting a little bit of data. You can search up the parents to find where you are in the tree, but it seems inefficient to have to do this on every plugin. But that seems to be the only way you could use this. Again, being able to do this on a per-namespace basis would seem more efficient.

You can also start from the top tree and traverse the tree in your modules by looking for changed flags as described here: https://clixon-docs.readthedocs.io/en/latest/backend.html#flags. But the callback need to keep track of which top-level symbols it has. Some users favor this model.

But I am open to adding a more high-level API more coupled to modules. I would assume one would register a plugin by modules/namespaces and then get localized info about what changed in that module. Maybe even with pointer to where in the YANG the change occurred.

[cminyard]

Ok, I have pushed up some patches that add something like what we have talked about. I agree with you, being able to see the whole tree would have and advantage for many applications, you wouldn't want to lose that capability. This change won't work if the changes between namespaces are not independent.

Basically, after xml_diff() is called, it scans the source and target trees and detects changed top-level elements that have a namespace that a module has registered against. It creates a new transaction_data item for those that have just those elements and stores it in the original transaction data. The code assumes that the source and target trees are in the same order except for top-level added and deleted elements.

When calling the plugin callbacks, it will see if the plugin has a namespace. If it does, it looks in the transaction data for transaction data items matching the namespace and calls the plugin callback for those.

I tried to marry simplicity and efficiency in this; this is the best I could come up with without reworking things a lot.

Some comments...

The namespace is set with clixon_add_plugin(). It's not in the api structure. You could add it to the api structure and it would simplify the interface a bit and be more natural, but that would change the structure, and you couldn't re-use the same api structure for different namespaces.

This is not extensively tested, I have done a basic test of the new function and run the test suite.

This change consolidates a lot of common code in the backend plugin callbacks. That's probably a good idea even without this change.

[cminyard]

I fixed the issue that showed up. I thought it was a base issue, but how all the tests work is a little confusing. Anyway, it's fixed.

There is an issue with test_datastore_multi.sh, but I was able to reproduce that without my patches, and it only happens occasionally. I get:

Test 35(35) [Check running subfile not changed]

Error in Test35 [Check running subfile not changed]:
Expected
Timestamp not changed

Received: 1717430952 != 1717430953

Diff between Expected and Received:
1,3c1,3
< 0000000   1   7   1   7   4   3   0   9   5   2       !   =       1   7
< 0000020   1   7   4   3   0   9   5   3  \n
< 0000031
---
> 0000000   T   i   m   e   s   t   a   m   p       n   o   t       c   h
> 0000020   a   n   g   e   d  \n
> 0000026

[olofhagsand]

?> There is an issue with test_datastore_multi.sh, but I was able to reproduce that without my patches, and it only happens occasionally. I get:

Yes, it is a known that happens now and then

[cminyard]

I reworked the patches a bit.

The first four patches are ones I consider to be "good ideas in general". They consolidate redundant code and things like that. One fixes what I think is a bug.

Per the discussions in https://github.com/clicon/clixon/issues/292, I'm beginning to think it is better to not add a namespace-based plugin to the main clixon library, since API changes will be required to make it work right. I can reuse a lot of the code written here to do that.

[cminyard]

Doing this as an external plugin. No need for most of these changes. I'll try to open another request with the generic improvements.