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

mgmtd, lib: remove batch ids from cfg apply reply #14609

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

idryzhov
Copy link
Contributor

@idryzhov idryzhov commented Oct 17, 2023

The config is always applied fully, all batches are included. There's no
need to pass a list of applied batches as it always contains all of
them.

The config is always applied fully, all batches are included. There's no
need to pass a list of applied batches as it always contains all of
them.

Signed-off-by: Igor Ryzhov <[email protected]>
@frrbot frrbot bot added libfrr mgmt FRR Management Infra labels Oct 17, 2023
@idryzhov idryzhov requested a review from choppsv1 October 17, 2023 12:07
@idryzhov idryzhov requested a review from pushpasis October 17, 2023 12:07
@github-actions github-actions bot added size/L and removed size/M labels Oct 21, 2023
@idryzhov idryzhov changed the title mgmtd, lib: remove batch ids from cfg apply reply mgmtd, lib: remove batch ids from messages Oct 21, 2023
@idryzhov
Copy link
Contributor Author

@choppsv1 @pushpasis I pushed additional commit to completely remove batch ids. It simplifies the code and also prevents possible race condition from happening.

The race condition happens when one of the backends is still stuck on the TXN_CREATE phase and another one has completed the SEND_CFG phase. In this case, batches of the latter backend are added to the next_batches list twice which leads to a crash later when the transaction is cleaned up.

@choppsv1
Copy link
Contributor

choppsv1 commented Oct 24, 2023

@idryzhov could you create a second PR for removing all batch IDs. Removing them in the initial commit is obvious. The second change is larger and I'd like to review it in a separate PR.

@idryzhov idryzhov force-pushed the cfg-apply-remove-batches branch from 51227d9 to d2977d5 Compare October 25, 2023 15:40
@github-actions github-actions bot added size/M and removed size/L labels Oct 25, 2023
@idryzhov idryzhov changed the title mgmtd, lib: remove batch ids from messages mgmtd, lib: remove batch ids from cfg apply reply Oct 25, 2023
@idryzhov
Copy link
Contributor Author

@choppsv1 I removed the second commit. I'll open a new PR once this one is merged.

@choppsv1 choppsv1 merged commit a709218 into FRRouting:master Oct 25, 2023
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants