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.

Sign up for GitHub

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

Jump to bottom

Fixes #1512 - Adds Commit Confirm support to IOS #1538

Closed
wants to merge 14 commits into from
Closed

Fixes #1512 - Adds Commit Confirm support to IOS #1538

wants to merge 14 commits into from

Conversation

DanSheps
Copy link
Contributor

I believe this is now ready for review.

Both merge and replace both support the functionality

@mirceaulinic mirceaulinic added this to the 3.5.0 milestone Feb 12, 2022
@mirceaulinic mirceaulinic modified the milestones: 3.5.0, 4.0.0 Feb 12, 2022
@ktbyers
Copy link
Contributor

ktbyers commented Feb 13, 2022

I will try to review and test this in the next 2 to 3 weeks.

@ktbyers ktbyers self-assigned this Mar 10, 2022
@DanSheps
Copy link
Contributor Author

DanSheps commented Apr 5, 2022

I see there are some black errors, I can go ahead and fix those up.

@Kircheneer Kircheneer mentioned this pull request Jun 9, 2022
Open
@DanSheps
Copy link
Contributor Author

DanSheps commented Jun 9, 2022

Perhaps @Kircheneer could you give this a once over?

Kircheneer
Kircheneer reviewed Jun 9, 2022
View reviewed changes
napalm/ios/ios.py Outdated
Comment on lines 526 to 529
"For Cisco devices revert_in between {} and {} seconds, this will round "
"down to the nearest minute".format(
CISCO_TIMER_MIN * 60, CISCO_TIMER_MAX * 60
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be a little clearer to say "For Cisco IOS devices revert_in is rounded down to the nearest minute, pass revert_in as a multiple of 60"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be okay with that

@Kircheneer
Copy link
Contributor

Sorry if this is obvious, but where exactly are we calling commit confirm? I can't find it for either the merge nor the replace branch. Apart from that LGTM.

@DanSheps
Copy link
Contributor Author

DanSheps commented Jun 9, 2022

Sorry if this is obvious, but where exactly are we calling commit confirm? I can't find it for either the merge nor the replace branch. Apart from that LGTM.

Line 648

@Kircheneer
Copy link
Contributor

If there is anything I can contribute to move this forward, please let me know. I would love to see this get merged. Many thanks!

@ktbyers
Copy link
Contributor

ktbyers commented Jun 15, 2022

My bad on this. I didn't get to reviewing this earlier.

Let me see if I can work on it in June.

ktbyers
ktbyers reviewed Jun 27, 2022
View reviewed changes
Copy link
Contributor

@ktbyers ktbyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to do some more review and testing, but this is a start.

napalm/ios/ios.py Outdated

def _check_archive_feature(self):
cmd = "show archive"
output = self.device.send_command_expect(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to send_command and not send_command_expect (as that is the more proper way in Netmiko to call this method and consistent with the other NAPALM code).

napalm/ios/ios.py Outdated
def _get_pending_commits(self):
if self._check_archive_feature():
cmd = "show archive config rollback timer"
output = self.device.send_command_expect(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use send_command and not send_command_expect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also _get_pending_commits() should return timer in seconds (as opposed to a string). This will make it more consistent with the behavior of the junos and eos driver.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind...we can just leave the timer as-is. That should be fine.

napalm/ios/ios.py Outdated
)
raise CommitConfirmException(msg)
else:
revert_in = int(revert_in / 60)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this revert_in_min or something that clearly differentiates it from the passed in argument. I say this as it would be easy to miss that the standard revert_in argument which is in seconds, has been converted to a different argument which is now in minutes form. Consequently, I think it makes sense to have a different name for this variable.

napalm/ios/ios.py Outdated
@@ -534,14 +566,25 @@ def commit_config(self, message="", revert_in=None):
msg = "Candidate config could not be applied\n{}".format(output)
raise ReplaceConfigException(msg)
elif "%Please turn config archive on" in output:
msg = "napalm-ios replace() requires Cisco 'archive' feature to be enabled."
raise ReplaceConfigException(msg)
raise CommitConfirmException(ARCHIVE_DISABLED_MESSAGE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a check for if revert_in here. In other words, probably should look something like:

            elif "%Please turn config archive on" in output:
                if revert_in:
                    raise CommitConfirmException(ARCHIVE_DISABLED_MESSAGE)
                else:
                    msg = "napalm-ios replace() requires Cisco 'archive' feature to be enabled."
                    raise ReplaceConfigException(msg)

NAPALM needs archive to be enabled regardless of whether you are using commit-confirm.

@ktbyers
Copy link
Contributor

ktbyers commented Jun 27, 2022

We need to update NAPALM documentation table also that indicates commit_confirm is now supported on Cisco IOS.

@DanSheps
Copy link
Contributor Author

DanSheps commented Jul 2, 2022

Thanks, I will work on this this coming week.

@ktbyers
Copy link
Contributor

ktbyers commented Jul 6, 2022

Okay, I ran these three tests in napalm_custom_test repository on Cisco IOS and they all passed.

&& py.test -s -v test_napalm_cfg.py::test_commit_confirm_revert --test_device ios \
&& py.test -s -v test_napalm_cfg.py::test_commit_confirm --test_device ios \
&& py.test -s -v test_napalm_cfg.py::test_commit_confirm_noconfirm --test_device ios \

@ktbyers
Copy link
Contributor

ktbyers commented Jul 6, 2022

@DanSheps Just let me know when you are done making updates and we can see if we can finish this PR off.

@DanSheps
Copy link
Contributor Author

DanSheps commented Jul 6, 2022

I think this is everything you requested.

@ktbyers
Copy link
Contributor

ktbyers commented Jul 6, 2022

Okay, sounds good...let me try to do final review and testing.

@ktbyers
Copy link
Contributor

ktbyers commented Jul 7, 2022

Also ran the following test:

py.test -s -v test_napalm_cfg.py::test_merge_commit_confirm --test_device ios

And it worked fine.

@ktbyers
Copy link
Contributor

ktbyers commented Jul 7, 2022

Superseded by:

#1691

@ktbyers ktbyers closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kircheneer Kircheneer Kircheneer left review comments

@ktbyers ktbyers ktbyers approved these changes

Assignees

@ktbyers ktbyers

Labels
commit confirm enhancement ios
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@DanSheps @ktbyers @Kircheneer @mirceaulinic