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

[WIP] lib, mgmtd: Fix the Scratch Buffer usage in Northbound code #14594

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pushpasis
Copy link
Contributor

The changes to the candidate data trees were not getting added to the scratch-buffer since lyd_diff_get_op was returning nothing ('n') as no 'operational' metadata associated with the data items in the YANG models. Fixed the logic to add them to the scratch buffer anyways.

Also, the scratch-buffer updations were getting invoked everywhere (mgmtd, backend clients). Limited the usage to mgmtd daemon only.

@idryzhov
Copy link
Contributor

idryzhov commented Oct 13, 2023

Did you test this? Just quickly looked at the code and I see several scenarios where I think it won't work. Please confirm that you've tested it on various scenarios (create/modify/delete data, leaves/containers, with existing and non-existing parents, etc.), before I actually start to review this.

It would be also great to see some numbers on performance difference between this and full diff.

@pushpasis pushpasis force-pushed the nb_scratchbuf_fix branch 2 times, most recently from 694d618 to 884fa22 Compare October 14, 2023 01:57
@pushpasis
Copy link
Contributor Author

Did you test this? Just quickly looked at the code and I see several scenarios where I think it won't work. Please confirm that you've tested it on various scenarios (create/modify/delete data, leaves/containers, with existing and non-existing parents, etc.), before I actually start to review this.

It would be also great to see some numbers on performance difference between this and full diff.

@idryzhov I did test quiet a few combination of scenarios... like..

  • create / delete / apply
  • create / apply / delete / apply
  • create / apply / modify / apply / delete / apply

In each case the extra log added in mgmt_txn.c was used to verify that we are using the scratch buffer. Also ran the mgmt topotests before uploading the PR. But haven't run the full TopoTests yet. Give me some time to finish those.

I will suggest you to start reviewing once all the topotests in CI runs without failure.

The changes to the candidate data trees were not getting added to
the scratch-buffer since lyd_diff_get_op was returning nothing ('n')
as no 'operational' metadata associated with the data items in
the YANG models. Fixed the logic to add them to the scratch buffer
anyways.

Also, the scratch-buffer updations were getting invoked everywhere
(mgmtd, backend clients). Limited the usage to mgmtd daemon only.

Signed-off-by: Pushpasis Sarkar <[email protected]>
Copy link

github-actions bot commented Nov 8, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@pushpasis pushpasis marked this pull request as draft November 20, 2023 09:17
@pushpasis pushpasis changed the title lib, mgmtd: Fix the Scratch Buffer usage in Northbound code {WIP] lib, mgmtd: Fix the Scratch Buffer usage in Northbound code Nov 20, 2023
@pushpasis pushpasis changed the title {WIP] lib, mgmtd: Fix the Scratch Buffer usage in Northbound code [WIP] lib, mgmtd: Fix the Scratch Buffer usage in Northbound code Nov 20, 2023
Copy link

This PR is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this PR closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants