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

Fix junos confirm-commit issue (improved version) #241

Merged
merged 3 commits into from
Jun 16, 2018

Conversation

attila123
Copy link

@attila123 attila123 commented Jun 15, 2018

Based on @ganeshrn fix (#239), but also clear the namespace for commit-configuration element (https://www.juniper.net/documentation/en_US/junos/topics/reference/tag-summary/junos-xml-protocol-commit-configuration.html) and update the corresponding unit tests.
I also tested it with a real Juniper SRX 300.

…he confirm timeout to seconds again to be compatible with ncclient's API. Also, do not use any XML namespace for the commit-configuration element, see https://www.juniper.net/documentation/en_US/junos/topics/reference/tag-summary/junos-xml-protocol-commit-configuration.html .
@coveralls
Copy link

coveralls commented Jun 15, 2018

Coverage Status

Coverage increased (+0.03%) to 69.044% when pulling 476639e on attila123:junos_confirm_commit_fix into 76d274b on ncclient:master.

@ganeshrn
Copy link
Contributor

imo the api should accept timeout value in minutes only as the junos rpc takes the value of timeout in minutes. With the current logic if the user input 121 sec it will end up waiting for rollback for 3 mins (ie. 180 sec) which seems incorrect to me.

@einarnn einarnn merged commit 46259c2 into ncclient:master Jun 16, 2018
@attila123
Copy link
Author

"imo the api should accept timeout value in minutes only as the junos rpc takes the value of timeout in minutes."
@ganeshrn the problem with this is that the ncclient library (and netconf) uses timeout in seconds, so if you don't provide seconds there, you break the API of ncclient library. The standard rpc for confirmed commit does not seem to work for Junos, so we are forced to wrap their proprietary and incompatible rpc to the standard API of ncclient, and convert the seconds to minutes in the best possible way.
IMHO, for practical purposes, one minute difference does not matter much for confirmed commit. Realistically, the timeout should be at least 10 minutes (default value in the protocol) to leave enough time for testing after configuration. But I am not a network engineer, do not have practical experience here. And it is better to leave one extra minute for the testing rather than timeout a bit earlier. To put it another way (this is the only thing I can imagine to support your opinion): in case of a totally bad config one might generate one extra minute of outage.
In any case thank you very much helping to fix this! :)

@attila123 attila123 deleted the junos_confirm_commit_fix branch June 18, 2018 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants