[sysrepo-devel] ?==?utf-8?q? new generation of sysrepo

[Michal Vaško]

Hello,

my input is inline.

On Wednesday, October 10, 2018 09:30 CEST, Radek Krejčí <rkrejci at cesnet.cz> wrote: 
 
> Hi Jan,
> as soon as michal will come back from his vacation, he'll surely add some more detailed answers, my opinions/understanding is inline...
> 
> Dne 02. 10. 18 v 17:07 Jan Kundrát napsal(a):
> > Hi,
> > here's a bunch of comments from my part. We're using sysrepo (and the rest of the NETCONF stack) as an exclusive management interface to our optical devices.
> >
> > I will start with a personal wishlist related to general SW development, testing and cross-compiling. I wish sysrepo supported self-contained, portable repository locations. That would be super-useful for unit tests. Each test could then run in complete isolation from other parallel executions. There would be no extra setup and teardown (maybe just a `cp` and `rm`). In other words, please allow programmers to specify a "prefix" or "context" when initializing the first connection, and locate *all* data underneath that prefix. It is OK to have a system-wide default for "production use", but please treat another prefix as a first-class citizen.
> 
> I'm not against this idea

Sounds reasonable and should be fairly easily implemented in the code I already have.

> >
> > Lack of SR_EV_VERIFY sounds like a blocker for us. We are trying to push as much stuff as possible to XPath-based `must` conditions in our own models, but we've been hitting performance problems in the past, and the models are only getting more and more complex. Also, there are existing models written by people who put the validation logic into their NETCONF servers, i.e., there are rules not visible to clients via YANG. We must implement these models, and SR_EV_VERIFY is an important tool for that. I see in the updated presentation that it is probably not going to be removed, just renamed -- that's great. We "just" need to be able to reject a proposed change.
> 
> as you write at the end - it should be just rename of what we currently have. The point is that now we have VERIFY and then APPLY. In many cases, the verification must be actually done by applying changes to the device. So we want the first notification to be APPLY followed by DONE/ABORT. Anyway, during the first notification, you will be still able to reject the changes, so the behavior is planned to be kept as it is now. We can preserve even the names and just more clarify the descriptions, but we prefer even renaming the notifications.

What Radek wrote is accurate. I just want to add that however it will look at the end, every use-case supported now will definitely remain supported.

> 
> >
> > I am a bit worried by the proposed use of SHM and zero-copy, but perhaps I'm just not understanding the slides. The current/legacy design uses GPB, a library for checking whether the data read from clients pass some rudimentory validation. How will different clients access SHM in the proposed next-gen design? Will they have direct access to, e.g., shared data structures? If the in-memory structures contain offsets or pointers, which part will check whether they are valid? What happens when a malicious client changes the data in an attempt to trigger a race condition (time of check vs time of use)?
> 
> with the proposed changes, we are actually moving to work more as /etc. The datastore (with granularity of the data for the different modules) access control will be done via standard Linux access rights (it is somehow already implemented in the current code). So only the processes with the allowed uid/gui will be allowed to access the specific data (the startup stored in files is clear, but the same mechanism should be used also for the shared memory segments). However, when the process has the access, it can do actually whatever it wants - which is the same case for the files in /etc.

Exactly, the same problem affects any process having access to an important configuration file, it can corrupt it. It is up to the system administrator to set file access rights correctly and to install only trustworthy applications.

> 
> >
> > Right now, notifications leave much to be desired. Either they are one-off, or they are stored in an extremely inefficient format which basically requires an O(n^2) parsing overhead [1]. I wonder if sysrepo is actually a correct place for storing high-volume notifications. Perhaps looking at some existing message-bus system might make sense?
> 
> Michal plans some changes, so it shouldn't be so demanding. Also with the design change of removing the main daemon, the processing demands will move to the specific processes which actually will work (store or load) with the notifications. However, looking how such a logging and working with a high-volume logs is done elsewhere, is a good point.

The notification implementation in the current sysrepo is very simplistic so I do not think that it is too difficult to write an efficient one if keeping this property in mind from the start. By that I mean storing notifications in O(1) (naturally, adding a notification to an empty file will be much faster than to a 10 MB one but this can also be solved) and loading them in O(n).

> 
> >
> > Ops data subscriptions -- currently there are some limitations on how the code can be structured and how the YANG model can look like, especially when it comes to lists. Our implementation requests data from some low-level HW modules, and batching requests provides a notable performance boost. Therefore I prefer a single request for all ops data for a given module (or perhaps even modules?) with a list of requested XPath subtrees. That way our low-level code can use the most efficient method for obtaining the data.
> >
> 
> I think that we were talking about some way to "cache" ops data, so you don't need to get each specific parameter from a device separately, but I'm not sure about the details right now. I think that there should be some announcement that we are now starting to get such data and then that we are done, so you can get all the data at the beginning and throw them out when it is done.

It is already possible to implement this with the (fairly) new request_id [3] parameter of the operational data callback. I mean to get all the data only once for one <get> request. What you are proposing (passing request XPaths and getting all the data in one callback) changes the whole mechanism significantly, which we are trying to avoid. Nevertheless, we can discuss this more when the time is right (too soon right now).

> 
> > Changes to the startup datastore. Right now it requires a convoluted setup [2] where we're using `inotify`, listening for changes to the `foo.startup` data file. This means that we cannot reject such a modification, and that we have to use an external tool rather than hooking into sysrepo. Our use case is simple, e.g., a plugin which handles the `ietf-interfaces` model and pregenrates some persistent config to be applied during next reboot.
> 
> I think that we were discussing that enhancing data subscriptions even to the startup repository is possible (despite I'm not a big fan of that).

Yeah, we are planning to support subscriptions not just for <running> but <startup> also.

> 
> >
> > Regarding locking, I don't have much to contribute here because our changes have so far only targeted a single DS. That said, it "feels wrong" if the commits are not actually atomic. If we can end up with a transaction which originally touches modules A and B, and it ends up only persisteng changes to, say, B, then that would be a bug in my opinion.
> 
> agree. The reason we want to have a separate locking for each YANG module data is to allow at least concurrent access to the different YANG module data. But when the changes affects more modules, all of them must be locked to start applying changes.

Nothing to add.

> 
> Thank you for your comments, I really appreciate them. As I wrote, I expect that Michal will add more details or will correct me during this week.
> 
> Regards,
> Radek
> 
> 
> >
> > Hope this helps,
> > Jan
> >
> > [1] https://github.com/sysrepo/sysrepo/issues/735
> > [2] https://github.com/sysrepo/sysrepo/issues/954
> > _______________________________________________
> > sysrepo-devel mailing list
> > sysrepo-devel at sysrepo.org
> > http://lists.sysrepo.org/listinfo/sysrepo-devel

Regards,
Michal

[3] https://github.com/sysrepo/sysrepo/blob/master/inc/sysrepo.h#L1847