-
Notifications
You must be signed in to change notification settings - Fork 555
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
Add support for configure private, without DB locking #749
Add support for configure private, without DB locking #749
Conversation
It looks like the config object supports using the private database, but only if called with a context manager: http://junos-pyez.readthedocs.io/en/2.1.8/jnpr.junos.utils.html#module-jnpr.junos.utils.config Do we know why the context manager is required? Can we leverage this instead of sending the rpc command directly? |
All the context manager does is handle issuing lock/unlock operations for the specific config mode -- should we just call them as part of |
Hi, And I think the common modes are private, exclusive, and configure. So private and exclusive are covered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me on the surface, though I'd like to test it before getting this in - mainly to ensure that the config DB is properly unlocked on discard / rollback / disconnect.
So how can we perform the checks? |
@omershtivi: Unfortunately this is a type of thing that cannot really be part of a test suite to be trusted enough, so I am going to test it effectively on a real box or VM in various scenarios. I hope to be able to get to this later today or next week. Cheers. |
@mirceaulinic I've tested the above (discarding, rollback, and disconnect) and everything worked fine. |
Hi, |
@omershtivi Can you give an overview of why you need this feature? What the use case would be? I have a sense of it, but I want to make sure my assumptions are not wrong: Main concerns here are:
I guess the other question is whether we (NAPALM) should even do this or whether this should just be outside of napalm i.e. we have a separate PR (#881) which allows you to control locking outside of napalm so if you want to do that, we provide a way (but then it is up to that system to ensure the unexpected error states are handled). I am probably inclined towards this last position, but am open to different arguments on this. |
Hi @ktbyers,
DB locking is just a portion of it, really need the private candidate file to avoid overlapping. |
Hi @omershtivi - there seem to be some conflicts, would you be able to resolve them? |
👋 @omershtivi - I've opened #1169 to revisit this, as I couldn't find the branch for this PR on your NAPALM fork. I've simply copied your changes, let me know if you'd prefer to continue working on this PR, re-submit another one, or go ahead with mine. 😉 |
Closing this in the favour of #1169. Thanks for your contribution @omershtivi, and sorry for not looking into this earlier as promised! 🙏 |
No description provided.