-
Notifications
You must be signed in to change notification settings - Fork 557
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
Closes #2005: Change rollback behavior #2006
Closes #2005: Change rollback behavior #2006
Conversation
d6c5609
to
5e4f948
Compare
Related issue and discussion: |
@decoupca Is there a separate change for Cisco IOS?
|
No, sounded like we wanted to make the changes in one pass but I can separate it out if you like. |
self.changed = False | ||
if not self._check_file_exists(cfg_file=self.rollback_cfg): | ||
msg = f"Rollback file '{self.rollback_cfg}' does not exist on device." | ||
raise ReplaceConfigException(msg) |
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 might not be the right exception i.e. what are the cases where rollback
gets called. We probably need to look at that a bit more. I generally view a ReplaceConfigException
as an exception you get when you try to do the replace config operation.
I guess we could raise a generic NapalmException
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.
I copied ReplaceConfigException
from other code that raises that exception during a rollback. I thought about raising NapalmException
or creating a new RollbackConfigException
, but I don't have strong preferences here.
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.
Okay, let me look some more. I only looked at it quickly (it could make the most sense given what we have previously done).
@decoupca There aren't any IOS changes in this pull-request though? Was there another file/change that needed included? |
Oh my mistake -- I didn't change anything, that check was already built in to the IOS driver. Only NXOS and NXOS_SSH have changed to match the IOS/JUNOS behavior. |
No worries...I just misunderstood (didn't check) what you had mentioned above. |
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.
LGTM
Changes rollback behavior so that calling
rollback()
attempts to roll back to the most recent config snapshot, regardless of whether napalm is aware of making any changes in the current session. For platforms that use file-based snapshots, ensures the rollback file exists on the remote system before attempting rollback.JUNOS, IOSXR and EOS all behaved this way before this PR -- no changes made.
IOS: added check to ensure rollback file exists before attempting.IOS already has a file exists check in place for rollbacksNXOS and NXOS-SSH: Removed check that prevented rollbacks if napalm is not aware of changes made. Also implemented
_check_file_exists
and added checks to ensurerollback_cfg
exists prior to rollback attempt.