Changes applying order during commit request after edit-config to candidate broken when request contains changes for a few modules about sysrepo HOT 14 OPEN

I found, that problem was gone when i rollback to sysrepo version 2.2.33 (a1075ab). The next commit (d081644) changes direct reading and writing into shm for subscription to ATOMIC operations and also add more checks for event presence in shm. I found this commit as last which changed sr_shmsub_multi_notify_write_event function.

from sysrepo.

If I understand it correctly, you want the changes of modules to be applied in a specific order. In current versions there is the function sr_module_change_set_order() that can explicitly set this order so just use that.

from sysrepo.

On the contrary, I'm waiting for edit-config request changes from the first post to be applied sequentially. Changes for ietf-interfaces - before changes for bbf-l2-forwarding (according they order in edit-config xml). Isn't it the default behaviour? Because we never had changes applying order broken on the previous sysrepo versions (not devel).
Thank you for your reply. I'll examine sr_module_change_set_order() signature and description.

from sysrepo.

The order of the data in the edit-config with regards to different modules is ignored since libyang v2 is used because the data are always ordered based on the modules order in the context, which is alphabetical. That is why you can explicitly set the module order.

from sysrepo.

Hello!
I'm writing regarding the problem related to the described above.

Problem: when execute an rpc to delete interfaces+forwarders - the request fails to delete subinterface as there is a reference to a forwarder.

<?xml version="1.0" encoding="utf-8"?>
<rpc xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" message-id="">
  <edit-config>
    <target>
      <running/>
    </target>
    <nc:config xmlns:nc="urn:ietf:params:xml:ns:netconf:base:1.0">
    <forwarding xmlns="urn:bbf:yang:bbf-l2-forwarding">
    <forwarders>
      <forwarder nc:operation="delete">
        <name>WAN1_T10_USER1_T10</name>
      </forwarder>
    </forwarders>
    <forwarding-databases>
      <forwarding-database nc:operation="delete">
        <name>WAN1_T10_USER1_T10</name>
      </forwarding-database>
    </forwarding-databases>
  </forwarding>
  <interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces">
    <interface nc:operation="delete">
            <name>USER1_T10</name>
    </interface>
    <interface nc:operation="delete">
            <name>WAN1_T10</name>
    </interface>
  </interfaces>
  </nc:config>
  </edit-config>
</rpc>

Meanwhile, if use sr_module_change_set_order to increase priority for bbf-l2-forwarding module for 10 instead of 0 - it fails to add forwarder as it is adding before subinterface. In the same time that interface/forwarder pair that was created before changing order can be successfully deleted

Sysrepo version is 2.2.150 (latest master).
libyang version is 2.1.144 (commit eecad552fe58326a646df9cc9d7fea145ed64e3b)

Is this intended? Shouldn't the order be reversed in case of delete operation? I can't find any info on the internet / in the code

If there is no such feature, could you please tell how can I get info about nc:operation='delete' in sr_modinfo_next_mod or what is the correct way to achieve this?

from sysrepo.

Problem: when execute an rpc to delete interfaces+forwarders - the request fails to delete subinterface as there is a reference to a forwarder.

Meaning you get a validation error and no data change occurs? That would be the correct behavior.

Meanwhile, if use sr_module_change_set_order to increase priority for bbf-l2-forwarding module for 10 instead of 0 - it fails to add forwarder as it is adding before subinterface. In the same time that interface/forwarder pair that was created before changing order can be successfully deleted

I do not understand what the order has to do with this, it should not matter at all for validation. But it is possible something does not work as it should, you may try using the current devel versions instead.

from sysrepo.

Thank you for your reply! I checked devel branch, unfortunately it didn't work

Just to clarify the question:

I want dependent modules to have reverse order when I delete their nodes in the same request. And sr_module_change_set_order does not count the difference between adding nodes (straight order) and removing nodes (reverse order)

Like if I want to add interfaces and add forwarders related to them in the same request, the order should be "add interface, add forwarder on that interface". But, if I want to delete that exact pair in one request, the order should be "delete the forwarder, delete the interface" (order is the opposite to add operation).

Does sysrepo have such behavior? How it is controlled, what part of the code is responsible for it so I could debug it in there.

If it does not, what would you recommend to implement it?

My implementation ideas:

• Use update event in our plugin to modify the data (not sure if we could change modules order there depending on nc:operation value in there)
• patch sysrepo to apply reverse order for 'delete' and 'remove' operations so sr_modinfo_next_mod would return correct order for them: read and split the request data in sr_shmsub_change_listen_process_module_events, and call callbacks for all nodes with removal operation, specifying that the order should be reversed and then call them for other operations

Also, it would work if configure each interface/forwarder in separate request, but it can't be implemented as a lot of our and customers testing configuration adds or deletes these objects in one request.

from sysrepo.

Does sysrepo have such behavior? How it is controlled, what part of the code is responsible for it so I could debug it in there.

No, the order cannot be controlled the way you described. Calling the callbacks in the right order is handled by sr_shmsub_change_notify_change() in shm_sub.c.

One way of implementing this would be subscribing the same callback to both interface and forwarder changes. The callback can then detect that both were removed or added and perform whatever it needs in the correct order.

from sysrepo.

One way of implementing this would be subscribing the same callback to both interface and forwarder changes. The callback can then detect that both were removed or added and perform whatever it needs in the correct order.

We have one change callback for all modules, it processes each module independently. Do you say we can process several modules at once? So we could apply the correct order

from sysrepo.

What you should be able to do is ignore whether sysrepo calls the callback for changes on bbf-l2-forwarding or ietf-interfaces. When getting the change iterator, you can specify XPath selecting any changes from the commit, not just from the particular module. So you should be able to switch the order this way but I have never tried it (and is not encouraged in general) so I cannot guarantee it will work.

from sysrepo.

I think we could just disable sorting of the incoming data tree to alphabetical order in libyang - so it would stay exactly as it is in the original request. Can you suggest places in the code to look at in libyang?

from sysrepo.

That is a bad idea because all the libyang code relies on that (and it is not alphabetical but following the schema node order). But if you still want to change it, look at lyd_insert_node() internal function.

from sysrepo.

Hello, thanks for your previous suggestions. Idea with changing libyang was abandoned, I failed find exact place of where this order is created, so it wasn't possible to disable it.
I tried reading operation of incoming nodes in sr_subscription_process_events but sr_module_change_set_order didn't work as it deadlocks at shm_mod.c:1699 in sr_shmmod_change_prio, even when I explicitly unlock it. Check out this code snippet

// function: sr_subscription_process_events
// line 4503
/* read all bytes from the pipe, there can be several events by now */
    do {
        ret = read(subscription->evpipe, buf, 1);
    } while (ret == 1);
    if ((ret == -1) && (errno != EAGAIN)) {
        SR_ERRINFO_SYSERRNO(&err_info, "read");
        sr_errinfo_new(&err_info, SR_ERR_INTERNAL, "Failed to read from an event pipe.");
        goto cleanup_unlock;
    }

    //////

    int should_reverse_order = 0;

    // similar to sr_shmsub_change_listen_process_module_events
    for (i = 0; i < subscription->change_sub_count; ++i) {
        if ((err_info = does_request_have_delete_op(&subscription->change_subs[i], subscription->conn, &should_reverse_order))) {
            goto cleanup_unlock;
        }
        if (should_reverse_order == 1) break; // if any node is about delete op - it is enough to reverse the order 
    }

    // I didn't check there if it actually unlocks or it returns error
    sr_rwunlock(&subscription->subs_lock, SR_SUBSCR_LOCK_TIMEOUT, SR_LOCK_READ, subscription->conn->cid, __func__);

    if (should_reverse_order) {
        // NOTE: The actual problem: it locks in there
        if(sr_module_change_set_order(subscription->conn , "bbf-l2-forwarding", SR_DS_RUNNING, 1)) {

So the question is: how can I set prio when the lock has been already set by this thread? I have lack of knowledge of how does sysrepo work internally, I didn't find anything about it in documentation.

In sr_shmmod_change_prio i see that shm_lock which provides access to module priority is filled with sr_shmmod_lock but then I lost understanding of how does it get filled and how can I use it in my situation.

// file shm_mod.c
// function sr_shmmod_change_prio
// line 1696
/* SHM MOD LOCK */
    if ((err_info = sr_shmmod_lock(ly_mod, ds, shm_lock, SR_CHANGE_CB_TIMEOUT, prio_p ? SR_LOCK_READ : SR_LOCK_WRITE,
            SR_CHANGE_CB_TIMEOUT, conn->cid, 0, ds_handle, 0))) {
        return err_info;
    }

from sysrepo.

I am sorry but I will not explain to you all the internals of sysrepo when you decided to modify something, that is not part of the support we provide.

from sysrepo.