Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes applying order during commit request after edit-config to candidate broken when request contains changes for a few modules #3210

Open
optimden opened this issue Dec 13, 2023 · 14 comments
Labels
is:question Issue is actually a question.

Comments

@optimden
Copy link

optimden commented Dec 13, 2023

Hello!
We have issue on sysrepo version 2.2.132 when run our functional tests. We send edit-config request to candidate datastore, which contains adding new subinterface objects and forwarders which are depend on these new subinterfaces:

<?xml version="1.0" encoding="UTF-8"?><rpc xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" message-id="urn:uuid:4996e15c-0f45-481a-b881-66cd63d668f1"><edit-config xmlns:nc="urn:ietf:params:xml:ns:netconf:base:1.0"><target><candidate/></target><config>
	<interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces">
		<interface nc:operation="replace">
            <name>USER1_T400</name>
			<type xmlns:bbfift="urn:bbf:yang:bbf-if-type">bbfift:vlan-sub-interface</type>
			<l2-dhcpv4-relay xmlns="urn:bbf:yang:bbf-l2-dhcpv4-relay">
				<enable>false</enable>
				<trusted-port>false</trusted-port>
			</l2-dhcpv4-relay>
            <dhcpv6-ldra xmlns="urn:bbf:yang:bbf-ldra">
				<enable>false</enable>
				<trusted-port>false</trusted-port>
			</dhcpv6-ldra>
			<interface-usage xmlns="urn:bbf:yang:bbf-interface-usage">
				<interface-usage>user-port</interface-usage>
			</interface-usage>
			<subif-lower-layer xmlns="urn:bbf:yang:bbf-sub-interfaces">
				<interface>eth0_0_3</interface>
			</subif-lower-layer>
			<inline-frame-processing xmlns="urn:bbf:yang:bbf-sub-interfaces">
				<ingress-rule>
				  <rule>
					<name>USER1_T400_to_WAN1_T10T400</name>
					<priority>10000</priority>
					<flexible-match>
					  <match-criteria xmlns="urn:bbf:yang:bbf-sub-interface-tagging">
						<tag>
						  <index>0</index>
						  <dot1q-tag>
							<tag-type xmlns:bbf-dot1qt="urn:bbf:yang:bbf-dot1q-types">bbf-dot1qt:c-vlan</tag-type>
							<vlan-id>400</vlan-id>
							<pbit>any</pbit>
							<dei>any</dei>
						  </dot1q-tag>
						</tag>
						<ethernet-frame-type>any</ethernet-frame-type>
					  </match-criteria>
					</flexible-match>
					<ingress-rewrite>
						<pop-tags xmlns="urn:bbf:yang:bbf-sub-interface-tagging">0</pop-tags>
						<push-tag xmlns="urn:bbf:yang:bbf-sub-interface-tagging">
							<index>0</index>
							<dot1q-tag>
								<tag-type>34984</tag-type>
								<vlan-id>10</vlan-id>
								<pbit-from-tag-index>0</pbit-from-tag-index>
								<dei-from-tag-index>0</dei-from-tag-index>
							</dot1q-tag>
						</push-tag>
					</ingress-rewrite>
				  </rule>
				</ingress-rule>
				<egress-rewrite>
				  <pop-tags xmlns="urn:bbf:yang:bbf-sub-interface-tagging">0</pop-tags>
				</egress-rewrite>
			</inline-frame-processing>
		</interface>
		<interface nc:operation="replace">
            <name>WAN1_T10T400</name>
			<type xmlns:bbfift="urn:bbf:yang:bbf-if-type">bbfift:vlan-sub-interface</type>
			<l2-dhcpv4-relay xmlns="urn:bbf:yang:bbf-l2-dhcpv4-relay">
				<enable>false</enable>
				<trusted-port>false</trusted-port>
			</l2-dhcpv4-relay>
            <dhcpv6-ldra xmlns="urn:bbf:yang:bbf-ldra">
				<enable>false</enable>
				<trusted-port>false</trusted-port>
			</dhcpv6-ldra>
			<interface-usage xmlns="urn:bbf:yang:bbf-interface-usage">
				<interface-usage>network-port</interface-usage>
			</interface-usage>
			<subif-lower-layer xmlns="urn:bbf:yang:bbf-sub-interfaces">
				<interface>eth1</interface>
			</subif-lower-layer>
			<inline-frame-processing xmlns="urn:bbf:yang:bbf-sub-interfaces">
				<ingress-rule>
				  <rule>
					<name>WAN1_T10T400_to_USER1_T400</name>
					<priority>10000</priority>
					<flexible-match>
						<match-criteria xmlns="urn:bbf:yang:bbf-sub-interface-tagging">
							<tag>
								<index>0</index>
								<dot1q-tag>
									<tag-type xmlns:bbf-dot1qt="urn:bbf:yang:bbf-dot1q-types">bbf-dot1qt:s-vlan</tag-type>
									<vlan-id>10</vlan-id>
									<pbit>any</pbit>
									<dei>any</dei>
								</dot1q-tag>
							</tag>
							<tag>
								<index>1</index>
								<dot1q-tag>
									<tag-type xmlns:bbf-dot1qt="urn:bbf:yang:bbf-dot1q-types">bbf-dot1qt:c-vlan</tag-type>
									<vlan-id>400</vlan-id>
									<pbit>any</pbit>
									<dei>any</dei>
								</dot1q-tag>
							</tag>
							<ethernet-frame-type>any</ethernet-frame-type>
					  </match-criteria>
					</flexible-match>
					<ingress-rewrite>
						<pop-tags xmlns="urn:bbf:yang:bbf-sub-interface-tagging">1</pop-tags>
					</ingress-rewrite>
				  </rule>
				</ingress-rule>
				<egress-rewrite>
				  <pop-tags xmlns="urn:bbf:yang:bbf-sub-interface-tagging">0</pop-tags>
				</egress-rewrite>
			</inline-frame-processing>
		</interface>
	</interfaces>
	<forwarding xmlns="urn:bbf:yang:bbf-l2-forwarding">
		<forwarders>
			<forwarder nc:operation="replace">
				<name>WAN1_T10T400_USER1_T400</name>
				<mac-learning>
					<forwarding-database>WAN1_T10T400_USER1_T400</forwarding-database>
				</mac-learning>
				<split-horizon-profiles>
					<split-horizon-profile>USER_PORTS_ISOLATION</split-horizon-profile>
				</split-horizon-profiles>
				<multicast xmlns="http://inango.com/forwarding">false</multicast>
				<l2-dhcpv4-relay xmlns="urn:bbf:yang:bbf-l2-dhcpv4-relay-forwarding">
					<downstream-broadcast-flooding>single-user-with-fallback-to-layer2</downstream-broadcast-flooding>
				</l2-dhcpv4-relay>
				<ports>
					<port>
						<name>WAN1_T10T400</name>
						<sub-interface>WAN1_T10T400</sub-interface>
					</port>
					<port>
						<name>USER1_T400</name>
						<sub-interface>USER1_T400</sub-interface>
					</port>
				</ports>
			</forwarder>
		</forwarders>
		<forwarding-databases>
			<forwarding-database nc:operation="replace">
				<name>WAN1_T10T400_USER1_T400</name>
				<max-number-mac-addresses>32</max-number-mac-addresses>
				<aging-timer>30</aging-timer>
			</forwarding-database>
		</forwarding-databases>
	</forwarding>
</config></edit-config></rpc>

Then we send commit message like this:

<?xml version="1.0" encoding="UTF-8"?><rpc xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" message-id="urn:uuid:57781a82-5748-4368-b0a1-42cd8b4f142d"><commit/></rpc>

We perform 4 such requests (with different subinterfaces and forwarders names) sequentially. But test always failed on second or third request due wrong changes applying order.
In case of successful handling request we haven't new SR_EV_CHANGE in shared memory for bbf-l2-forwarding module when we handle SR_EV_CHANGE for ietf-interfaces:

Publishing part:
Dec 12 07:28:26 netopeer2-server[18520]: [INF]: SR: EV ORIGIN: "ietf-interfaces" "change" ID 1 priority 0 for 2 subscribers published.
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_subscription_process_events: &subscription->change_subs[i]->module_name = IF-MIB
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: begin
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /IF-MIB:IF-MIB, request_id = 0, priority = 0, sub->request_id = 0, sub->priority = 0
Dec 12 07:28:26 netopeer2-server[18520]: sysrepo: ### sr_shmsub_multi_notify_write_event: orig_name = netopeer2
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /IF-MIB:IF-MIB - not a listener event
Dec 12 07:28:26 netopeer2-server[18520]: sysrepo: ### sr_shmsub_multi_notify_write_event: multi_sub_shm->request_id = 1, multi_sub_shm->priority = 0
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: i = 1, change_subs->sub_count = 1
Dec 12 07:28:26 netopeer2-server[18520]: sysrepo: ### sr_shmsub_multi_notify_write_event: remap
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: no new module event
Dec 12 07:28:26 netopeer2-server[18520]: sysrepo: ### sr_shmsub_multi_notify_write_event: shm_data_sub->addr = 0x56217e1c
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_subscription_process_events: &subscription->change_subs[i]->module_name = bbf-l2-dhcpv4-relay
Dec 12 07:28:26 netopeer2-server[18520]: sysrepo: ### sr_shmsub_multi_notify_write_event: orig_size != NULL
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: begin
Dec 12 07:28:26 netopeer2-server[18520]: EV ORIGIN: "bbf-l2-forwarding" "change" ID 1 priority 0 for 2 subscribers published.
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-dhcpv4-relay:l2-dhcpv4-relay-profiles, request_id = 0, priority = 0, sub->request_id = 0, sub->priority = 0
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-dhcpv4-relay:l2-dhcpv4-relay-profiles - not a listener event
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: i = 1, change_subs->sub_count = 1
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: no new module event

Handling part:
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_subscription_process_events: &subscription->change_subs[i]->module_name = bbf-l2-forwarding
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: begin
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-forwarding:forwarding-state, request_id = 1, priority = 0, sub->request_id = 0, sub->priority = 0
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-forwarding:forwarding-state - not a listener event
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-forwarding:forwarding, request_id = 1, priority = 0, sub->request_id = 0, sub->priority = 0
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-forwarding:forwarding - not a listener event
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: i = 2, change_subs->sub_count = 2
Dec 12 07:28:26 sysrepo-plugind[18485]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: no new module event

But in falied case sysrepo added SR_EV_CHANGE into shm for bbf-l2-forwarding before we handle bbf-l2-forwarding module when we handle SR_EV_CHANGE for ietf-interfaces. So, changes for bbf-l2-forwarding depended from ietf-interfaces module changes are applied firstly. We can't find necessary new subinterfaces to create forwarders based on them and request fails:

Publishing part:
Dec 12 09:32:11 netopeer2-server[19060]: EV ORIGIN: "ietf-interfaces" "change" ID 1 priority 0 for 2 subscribers published.
Dec 12 09:32:11 netopeer2-server[19060]: [INF]: SR: EV ORIGIN: "ietf-interfaces" "change" ID 1 priority 0 for 2 subscribers published.
Dec 12 09:32:11 netopeer2-server[19060]: sysrepo: ### sr_shmsub_multi_notify_write_event: multi_sub_shm = 0x74c57000, request_id = 1, event = 4
Dec 12 09:32:11 netopeer2-server[19060]: sysrepo: ### sr_shmsub_multi_notify_write_event: multi_sub_shm->request_id = 1, multi_sub_shm->priority = 0
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_subscription_process_events: &subscription->change_subs[i]->module_name = IF-MIB
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: begin
Dec 12 09:32:11 netopeer2-server[19060]: sysrepo: ### sr_shmsub_multi_notify_write_event: remap
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: multi_sub_shm = 0x77fb4000, event = 0, sub->xpath = /IF-MIB:IF-MIB, request_id = 0, priority = 0, sub->request_id = 0, sub->priority = 0
Dec 12 09:32:11 netopeer2-server[19060]: sysrepo: ### sr_shmsub_multi_notify_write_event: shm_data_sub->addr = 0x5631798c
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /IF-MIB:IF-MIB - not a listener event
Dec 12 09:32:11 netopeer2-server[19060]: sysrepo: ### sr_shmsub_multi_notify_write_event: orig_size != NULL
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: i = 1, change_subs->sub_count = 1
Dec 12 09:32:11 netopeer2-server[19060]: EV ORIGIN: "bbf-l2-forwarding" "change" ID 1 priority 0 for 2 subscribers published.

Handling part:
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_subscription_process_events: &subscription->change_subs[i]->module_name = bbf-l2-forwarding
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: begin
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: multi_sub_shm = 0x7778f000, event = 4, sub->xpath = /bbf-l2-forwarding:forwarding-state, request_id = 1, priority = 0, sub->request_id = 0, sub->priority = 0
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-forwarding:forwarding-state - NEW EVENT
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: break on /bbf-l2-forwarding:forwarding-state
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_process_module_events: i = 0, change_subs->sub_count = 2
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: multi_sub_shm = 0x7778f000, event = 4, sub->xpath = /bbf-l2-forwarding:forwarding-state, request_id = 1, priority = 0, sub->request_id = 0, sub->priority = 0
Dec 12 09:32:11 sysrepo-plugind[19032]: [INF] sysrepo: ### sr_shmsub_change_listen_is_new_event: sub->xpath = /bbf-l2-forwarding:forwarding-state - NEW EVENT
Dec 12 09:32:11 netopeer2-server[19060]: [INF]: SR: sysrepo: ### sr_shmsub_multi_notify_write_event: multi_sub_shm = 0x74c57000, request_id = 1, event = 4
Dec 12 09:32:11 netopeer2-server[19060]: [INF]: SR: sysrepo: ### sr_shmsub_multi_notify_write_event: multi_sub_shm->request_id = 1, multi_sub_shm->priority = 0
Dec 12 09:32:11 netopeer2-server[19060]: [INF]: SR: sysrepo: ### sr_shmsub_multi_notify_write_event: remap
Dec 12 09:32:11 netopeer2-server[19060]: [INF]: SR: sysrepo: ### sr_shmsub_multi_notify_write_event: shm_data_sub->addr = 0x5631798c
Dec 12 09:32:11 netopeer2-server[19060]: [INF]: SR: sysrepo: ### sr_shmsub_multi_notify_write_event: orig_size != NULL
Dec 12 09:32:11 netopeer2-server[19060]: [INF]: SR: EV ORIGIN: "bbf-l2-forwarding" "change" ID 1 priority 0 for 2 subscribers published.

I got such backtrace for corresponding pieces of code:

sr_subscription_process_events --->

/* change subscriptions */
    for (i = 0; i < subscription->change_sub_count; ++i) {
        if ((err_info = sr_shmsub_change_listen_process_module_events(&subscription->change_subs[i], subscription->conn))) {
            goto cleanup_unlock;
        }
    }
  
---> sr_shmsub_change_listen_process_module_events
            |
        sr_shmsub_change_listen_is_new_event --->
   
    sr_sub_event_t event = ATOMIC_LOAD_RELAXED(multi_sub_shm->event);  <----- here we got difference values for bbf-l2-forwarding shm for successful and failed cases
    uint32_t request_id = ATOMIC_LOAD_RELAXED(multi_sub_shm->request_id);
    uint32_t priority = ATOMIC_LOAD_RELAXED(multi_sub_shm->priority);

    /* not a listener event */
    if (!SR_IS_LISTEN_EVENT(event)) {
        return 0;
    }
        

So, when we handle SR_EV_CHANGE for ietf-interfaces, we check new events for bbf-l2-forwarding module also (before we check ietf-interfaces, due bbf-l2-forwarding is first in change_subs array).

But it worked fine on sysrepo 2.1.64 version.

@optimden
Copy link
Author

optimden commented Dec 14, 2023

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.

@michalvasko
Copy link
Collaborator

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.

@michalvasko michalvasko added the is:question Issue is actually a question. label Dec 14, 2023
@optimden
Copy link
Author

optimden commented Dec 14, 2023

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.

@michalvasko
Copy link
Collaborator

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.

@probitie
Copy link

probitie commented Feb 27, 2024

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?

@michalvasko
Copy link
Collaborator

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.

@probitie
Copy link

probitie commented Feb 28, 2024

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.

@michalvasko
Copy link
Collaborator

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.

@probitie
Copy link

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

@michalvasko
Copy link
Collaborator

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.

@probitie
Copy link

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?

@michalvasko
Copy link
Collaborator

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.

@probitie
Copy link

probitie commented Mar 7, 2024

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;
    }

@michalvasko
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:question Issue is actually a question.
Projects
None yet
Development

No branches or pull requests

3 participants