don't remove file prompt quiet unnecessarily #570

benmaddison · 2017-12-01T17:04:04Z

If file prompt quiet is already present in the config at the start of an operation, napalm shouldn't remove it.
This is fixed by checking the running-config during _disable_confirm(), and setting a flag (IOSDriver._do_enable_confirm) to indicate whether the configuration line ought to be removed during later calls to _enable_confirm().
A RuntimeError is raised if the attribute does not exist, since this indicates that the methods were called in the incorrect order.
The configuration line is also removed from ignore_strings within _normalize_compare_config() since this line will only be present in the diff when it is included in either the candidate and/or (existing) running configs. In that case, a change in that command should show up in coompare_config().

coveralls · 2017-12-01T17:07:04Z

Coverage decreased (-0.1%) to 78.456% when pulling 3de7077 on benmaddison:develop into 584197e on napalm-automation:develop.

coveralls · 2017-12-02T10:47:39Z

Coverage decreased (-0.1%) to 78.459% when pulling c79a6a2 on benmaddison:develop into 890b936 on napalm-automation:develop.

ktbyers · 2017-12-02T16:49:06Z

@benmaddison I am somewhat against tracking state of this as it is prone to problems.

Your solution would also need need to check the current status of the flag (self._do_enable_confirm) otherwise multiple calls to _disable_confirm() to get into the wrong state.

I would rather just have an __init__ optional_args of enable_confirm=True and if you passed this in as False, then it would never toggle enable/disable confirm.

itdependsnetworks · 2017-12-02T20:05:53Z

I don't think it is obvious to the end user that you are adjusting the config on their behalf. I'm inclined to agree with @benmaddison

ktbyers · 2017-12-02T20:45:41Z

We have to enable this automatically (otherwise the napalm-ios code will just break and then there will be a lot of issues created and it will be very difficult to manage).

So the solutions I see:

Track the state and try to restore it to original state (which I am against as it is problematic to do this reliably).**
Give people a way to disable this behavior via an optional argument.
Do nothing and let people add a call to restore it in their code.

I think item 2 is a reasonable solution...but feel free to chime in.

There are certain prerequisites to automation using screen-scraping and one of them is to turn off the things that make automation incredibly hard (like prompting for additional things).

**Tracking state is very hard as you can't ensure you restore the state properly. Basically abnormal closures of the script will cause the state not to be restored properly.

dbarrosop · 2017-12-03T21:00:05Z

what about having this as a requirement as we have for the archive functionality?

ktbyers · 2017-12-03T22:30:45Z

@dbarrosop

I am concerned about debugging/troubleshooting failures associated with this (so I think we should automatically do it). I think the side-effects are quite a bit more benign than automatically enabling the archive functionality (which is why I don't think we should do that automatically).

I think we should give people a way to turn this behavior off via an optional_args and that should be sufficient.

Note, if you are not using inline transfers, NAPALM-IOS also automatically enables ip scp server enable and does not disable it (this statement only applies for NAPALM configuration operations). This is a more significant change than file prompt quiet.

I guess we could do something like check this setting immediately at open() and terminate NAPALM if it is not set right with a message and do a similar check for any config operations (for both archive and SCP configuration).

Although I think I would probably rather have config side effects (with the option of disabling them) and make the library easier to use and maintain (than not having config side effects).

Anyways we shouldn't do what is proposed in the PR which is trying to track the state of this setting.

ktbyers · 2017-12-03T22:33:16Z

I would say we open up a separate issue on napalm-ios configuration side-effects which includes both ip scp server enable and 'file prompt quiet` and decide what to do about them in that issue.

ktbyers · 2017-12-03T23:21:16Z

I think we should probably change the default on that ip scp server enable and not automatically enable it (i.e. treat it the same way as archive).

benmaddison · 2017-12-04T06:15:37Z

@ktbyers your point regarding state tracking is fair. This can be made safer using a method decorator that implements the enable; do a file operation; disable logic rather than having separate methods that could be called out of order. There is no way to track that state across runs if we encounter (for example) transport errors during do a file operation though.
Providing a kwarg to __init__ is a non-starter for me, as one would need to call the constructor differently depending on the desired final config of the managed device, which rather defeats the whole purpose of using Napalm in the first place.
I have no problem with making file prompt quiet a pre-requisite, and agree that this is probably the simplest option. The downside to this is that people may have existing tooling (expect scripts, etc) that assumes file operations will be prompted for, and this would break things for those people.
In general config pre-reqs should not be enabled auto-magically, nor removed silently - i.e. they should all be handled like the archive requirement.
It would be great if we could agree on a solution to this as soon as possible though, as I am currently having to run my fork in production :-(

ktbyers · 2017-12-04T07:41:48Z

@benmaddison tracking state is too problematic...so I think we can scratch that option off the list. And I think it probably isn't a pattern NAPALM should do (in general).

I didn't understand this below statement?

Providing a kwarg to __init__ is a non-starter for me, as one would need to call 
the constructor differently depending on the desired final config of the managed 
device, which rather defeats the whole purpose of using Napalm in the first place.

Maybe you are misunderstanding what I was proposing for the optional argument? The optional argument would just cause NAPALM to not modify 'file prompt' at all. NAPALM would neither enable it nor disable it (i.e. you would have to have your device configured already for 'file prompt quiet'). So you would pass this same optional argument in for all your devices. If this argument wasn't used, it would do what it currently does.

The downside to this is that people may have existing tooling (expect scripts, etc) 
that assumes file operations will be prompted for, and this would break things for 
those people.

I think the above is acceptable. There is only so much you can do.

In general config pre-reqs should not be enabled auto-magically, nor removed 
silently - i.e. they should all be handled like the archive requirement.

I think I disagree with this to a certain extent (though I understand the sentiment). There is a trade-off in how reliable easy to use the library is versus changing things. For some devices it is a config change to disable output paging. This prompting for additional information is really a terminal characteristic (unfortunately it is one that is retained across time). Or more exactly it should be a VTY characteristic and Cisco chose to not make it one.

Anyways we will need to decide on what to do on this and also on 'ip scp server enable'.

dbarrosop · 2017-12-04T15:52:43Z

In general config pre-reqs should not be enabled auto-magically, nor removed silently - i.e. they should all be handled like the archive requirement.

I tend to agree with this statement and hence my proposal before but I also agree/understand what ktbyers is saying and sometimes you have to balance usability with cleanness.

The optional argument would just cause NAPALM to not modify 'file prompt' at all

This is probably a good compromise

benmaddison · 2017-12-06T13:33:57Z

I'm not at all convinced that we should be touching configs to facilitate operations under the hood unless we have absolutely no choice (terminal length 0 can be run at exec to modify the current vty, just BTW). However, the compromise solution fixes my immediate issue, so I can live with it.
Happy to write the changes if that would be useful?

ktbyers · 2017-12-06T17:19:09Z

@benmaddison On IOS you can change terminal length 0 just for the VTY...on other platforms that is not necessarily true. So this is more a comment for Netmiko, but it applies to the conversation in general.

Yes, if you want to do a PR...sounds good.

I think we agreed to:

Change disable_confirm / enable_confirm so that it can be controlled via an optional argument. It will default to enabled (i.e. to the current behavior).
Change 'ip scp server enable' so that it doesn't get automatically changed (i.e. it will behave the same as 'archive'...you will have to manually configure it in advance).
Update documentation to reflect new optional argument and to make it clearer on the IOS caveats page that we do this config change.

ktbyers · 2017-12-06T17:21:20Z

@bewing @mirceaulinic I will let you guys chime in...I totally understand the sentiment of not wanting any configuration changes to happen behind the scenes, but am also a bit concerned at additional troubleshooting this might involve (not automatically doing this).

So let us know if you have an opinion on it.

coveralls · 2017-12-08T13:10:32Z

Coverage decreased (-0.1%) to 78.489% when pulling 804d517 on benmaddison:develop into d29b15e on napalm-automation:develop.

coveralls · 2017-12-09T08:15:14Z

Coverage decreased (-0.04%) to 78.58% when pulling 1703f10 on benmaddison:develop into d29b15e on napalm-automation:develop.

benmaddison · 2017-12-09T08:23:14Z

@ktbyers I've added the auto_file_prompt argument to control auto-toggling of file prompt quiet.
The _disable_confirm() and _enable_confirm() are refactored into a method decorator (_file_operation()) that can be used to wrap methods that perform file ops.
The docs have been updated, except that someone needs to change XXX to the correct value in line 119 of docs/ios.rst before a release gets tagged.
I'll do a separate PR for the SCP change.

ktbyers · 2017-12-09T20:25:54Z

napalm/ios/ios.py

@@ -377,6 +382,33 @@ def compare_config(self):

        return diff.strip()

+    def _file_operation(f):


Let's rename this to something like _file_prompt_quiet. _file_operation is too generic.

ktbyers · 2017-12-09T20:26:30Z

docs/support/ios.rst

+File Operation Prompts
+_____
+
+By default IOS will prompt for confirmation on file operations. These prompts need to be disabled before the NAPALM-ios driver requires these to be disabled.


Second sentence doesn't make sense?

Hahaha, no, it really doesn't. I'll fix it...

ktbyers · 2017-12-09T20:28:12Z

docs/support/ios.rst

+_____
+
+By default IOS will prompt for confirmation on file operations. These prompts need to be disabled before the NAPALM-ios driver requires these to be disabled.
+This can be controlled using the `auto_file_prompt` optional arguement from version XXX:


Drop the version XXX as we will probably not remember to update this at the release time (you can add this post release if you want i.e. when you know the version that this change happened in).

ktbyers · 2017-12-09T20:36:27Z

napalm/ios/ios.py

+        self._discard_config()
+
+    @_file_operation
+    def _discard_config(self):


Why did you need to make a shadow method here (as opposed to just decorating discard_config)?

This was an interesting py2 vs py3 issue that I hadn't seen before:
In 2.7, functools.wraps() doesn't fix the signature of wrapper() to match the original function.
As a result, test.ios.test_getters.TestGetter.test_method_signatures() fails. Since that test skips private methods the easiest way to work-around was to decorate a private method and call it from the public one.
If you're aware of a more elegant fix then I'm happy to incorporate that?

Okay, makes sense. No I am not aware of a better solution...so we should just use your solution.

coveralls · 2017-12-10T07:14:59Z

Coverage decreased (-0.04%) to 78.58% when pulling 7062aa9 on benmaddison:develop into d29b15e on napalm-automation:develop.

coveralls · 2017-12-11T07:50:38Z

Coverage decreased (-0.05%) to 78.567% when pulling f054052 on benmaddison:develop into d29b15e on napalm-automation:develop.

ktbyers

Also tested on real IOS device both with auto_prompt_file True and False.

coveralls · 2017-12-12T20:04:42Z

Coverage decreased (-0.05%) to 78.727% when pulling 532a747 on benmaddison:develop into 5fdcfd1 on napalm-automation:develop.

don't remove file prompt quiet unnecessarily

3de7077

Merge branch 'develop' into develop

c79a6a2

Merge branch 'develop' into develop

804d517

make automatic file prompt handling optional

1703f10

ktbyers reviewed Dec 9, 2017

View reviewed changes

fix review items

7062aa9

preserve return values in wrapped methods

f054052

Merge branch 'develop' into develop

532a747

ktbyers approved these changes Dec 12, 2017

View reviewed changes

ktbyers merged commit 689909b into napalm-automation:develop Dec 12, 2017

ktbyers mentioned this pull request Dec 12, 2017

Cisco IOS driver should not automatically change the config #585

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't remove file prompt quiet unnecessarily #570

don't remove file prompt quiet unnecessarily #570

benmaddison commented Dec 1, 2017

coveralls commented Dec 1, 2017 •

edited

coveralls commented Dec 2, 2017 •

edited

ktbyers commented Dec 2, 2017

itdependsnetworks commented Dec 2, 2017

ktbyers commented Dec 2, 2017

dbarrosop commented Dec 3, 2017

ktbyers commented Dec 3, 2017

ktbyers commented Dec 3, 2017

ktbyers commented Dec 3, 2017

benmaddison commented Dec 4, 2017 •

edited

ktbyers commented Dec 4, 2017 •

edited

dbarrosop commented Dec 4, 2017

benmaddison commented Dec 6, 2017

ktbyers commented Dec 6, 2017

ktbyers commented Dec 6, 2017

coveralls commented Dec 8, 2017 •

edited

coveralls commented Dec 9, 2017 •

edited

benmaddison commented Dec 9, 2017

ktbyers Dec 9, 2017

benmaddison Dec 10, 2017

ktbyers Dec 9, 2017

benmaddison Dec 10, 2017

ktbyers Dec 9, 2017

benmaddison Dec 10, 2017

ktbyers Dec 9, 2017 •

edited

benmaddison Dec 10, 2017

ktbyers Dec 10, 2017

coveralls commented Dec 10, 2017 •

edited

coveralls commented Dec 11, 2017 •

edited

ktbyers left a comment

coveralls commented Dec 12, 2017 •

edited

		@@ -377,6 +382,33 @@ def compare_config(self):

		return diff.strip()

		def _file_operation(f):

don't remove file prompt quiet unnecessarily #570

don't remove file prompt quiet unnecessarily #570

Conversation

benmaddison commented Dec 1, 2017

coveralls commented Dec 1, 2017 • edited

coveralls commented Dec 2, 2017 • edited

ktbyers commented Dec 2, 2017

itdependsnetworks commented Dec 2, 2017

ktbyers commented Dec 2, 2017

dbarrosop commented Dec 3, 2017

ktbyers commented Dec 3, 2017

ktbyers commented Dec 3, 2017

ktbyers commented Dec 3, 2017

benmaddison commented Dec 4, 2017 • edited

ktbyers commented Dec 4, 2017 • edited

dbarrosop commented Dec 4, 2017

benmaddison commented Dec 6, 2017

ktbyers commented Dec 6, 2017

ktbyers commented Dec 6, 2017

coveralls commented Dec 8, 2017 • edited

coveralls commented Dec 9, 2017 • edited

benmaddison commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktbyers Dec 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 10, 2017 • edited

coveralls commented Dec 11, 2017 • edited

ktbyers left a comment

Choose a reason for hiding this comment

coveralls commented Dec 12, 2017 • edited

coveralls commented Dec 1, 2017 •

edited

coveralls commented Dec 2, 2017 •

edited

benmaddison commented Dec 4, 2017 •

edited

ktbyers commented Dec 4, 2017 •

edited

coveralls commented Dec 8, 2017 •

edited

coveralls commented Dec 9, 2017 •

edited

ktbyers Dec 9, 2017 •

edited

coveralls commented Dec 10, 2017 •

edited

coveralls commented Dec 11, 2017 •

edited

coveralls commented Dec 12, 2017 •

edited