Add commit confirmed feature #124
Conversation
mirceaulinic
force-pushed
the
commit-confirmed
branch
from
9581789
to
71383f1
Compare
mirceaulinic
force-pushed
the
commit-confirmed
branch
from
c587a4b
to
7920b53
Compare
|
I have tested and it seems to work as expected: >>> i.load_merge_candidate(config='ntp server 1.2.3.6')
>>> i.commit_config(confirmed=1)produces the logs: And indeed reverted the change I requested. Confirming the commit: >>> i.load_merge_candidate(config='ntp server 1.2.3.6')
>>> i.commit_config(confirmed=1)
>>> i.commit_confirm()produces the logs: and saved the running config. |
|
LGTM but I will defer to @ktbyers as he is our resident expert on everything ios. |
| @@ -354,6 +354,8 @@ def commit_config(self): | |||
| raise ReplaceConfigException("Candidate config file does not exist") | |||
| if self.auto_rollback_on_error: | |||
| cmd = 'configure replace {} force revert trigger error'.format(cfg_file) | |||
| elif confirmed: | |||
| cmd = 'configure replace bootflash:{} force time {}'.format(cfg_file, confirmed) | |||
There was a problem hiding this comment.
Don't hard-code the file system bootflash. The cfg_file variable already has the file_system which either gets auto detected or set by optional_args.
There was a problem hiding this comment.
I have hardcoded as I have noticed that it wasn't able to detect the file system and for this command it seems that is mandatory -- but I will double check this & fix.
There was a problem hiding this comment.
Netmiko should automatically try to autodectect by doing a dir. If this fails, however, you can still set it using NAPALM optional_args.
We should create an issue, however, if you are seeing this fail.
There was a problem hiding this comment.
The autodetect happens right as part of the open code in the NAPALM code (IIRC).
There was a problem hiding this comment.
Most probably I missed something. I will check.
| # Replace running config with the rollback config | ||
| # And schedule the revert time | ||
| # If not confirmed, it will go back to the current running-config state | ||
| cmd = 'configure replace bootflash:{} force time {}'.format(self.rollback_cfg, |
There was a problem hiding this comment.
Don't hard-code file system. Use _gen_full_path(self.rollback_cfg) to generate full path using file-system bound to object.
There was a problem hiding this comment.
We probably should ensure that the rollback_cfg exactly matches running config at this point.
There was a problem hiding this comment.
We can probably copy it just before to be sure?
There was a problem hiding this comment.
How about copy and then make sure no diffs (and throw an error if differences are found).
pynet-rtr1#copy system:running-config flash:rollback_config.txt
pynet-rtr1#show archive config differences system:running-config flash:rollback_config.txt
!Contextual Config Diffs:
!No changes were found
There was a problem hiding this comment.
Yes, that makes sense to me... didn't know about this command (as many other tricks I have discovered reading the code 😄 ).
|
@mirceaulinic Nice! I added some comments (mainly pertaining to the hard-coding of the file system). Also I would like to test this prior to merging. |
|
We also need unit tests for this on real devices (i.e. not the mocked unit tests / the real unit tests). |
|
I have tested this on a 2811 so I can run the tests after we implement them, no problem. |
|
Yes, I will probably try to work on the tests also...though it might be the end of next week. |
|
This will be reimplemented post-reunification. |
Based on the suggestions from #121, submitting this changes to try an implementation for commit confirmed.
For load_merge, when confirmed is required, a candidate config file is saved on the flash that is used afterwards to replace and schedule the rollback time.