Retries and configure private #1

autonomouse · 2017-10-05T09:59:18Z

Added "config_mode_cmd='configure private'" to the send_config_set command for use with juniper switches.

Unfortunatley, this has to include a temporary hack to allow the config mode to be set to "configure private", because there is an output = self.config_mode() in send_config_set. This will override any external call to config_mode which is required to set it to 'configure private'. The real fix is needed upstream in the netmiko codebase, but until such time as that PR is accepted, this hack is necessary.

… (tmp until patch approved upstream)

JohnGarbutt · 2017-10-05T10:26:20Z

networking_generic_switch/devices/netmiko_devices/__init__.py

            LOG.debug("Nothing to execute")
            return
-
+        for count in range(num_retries):


Why do we need the multiple retries? I think I missed that bit of the conversation.

JohnGarbutt · 2017-10-05T10:27:29Z

networking_generic_switch/devices/netmiko_devices/__init__.py

-                                                       error=e)
-        LOG.debug(output)
+            if not final_attempt:
+                LOG.error(e)


I think technically this this wants to be a warning log, would be an error log on the final error I think.

JohnGarbutt · 2017-10-05T10:28:10Z

networking_generic_switch/devices/netmiko_devices/__init__.py

            with netmiko.ConnectHandler(**self.config) as net_connect:
                net_connect.enable()
-                output = net_connect.send_config_set(config_commands=cmd_set)
+                output = net_connect.send_config_set(


I would be tempted to split this into two commits, just so we have the history of each change described in the commit message, for when we come back to this.

JohnGarbutt · 2017-10-05T11:01:23Z

networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py

                                        ip='host')
-        connect_mock.send_config_set.assert_called_once_with(
-            config_commands=['spam ham aaaa'])
+        # FIXME: uncommented out once netmiko patch is accepted upstream:


Not sure I see any differences here? Which seems wrong I guess.

autonomouse · 2017-10-05T11:09:40Z

networking_generic_switch/devices/netmiko_devices/__init__.py

+    output = net_connect._sanitize_output(output)
+    return output
+
+


This entire method is lifted from netmiko, with the only alterations being the selfs being changed to net_connects, config_mode now accepting config_mode_cmd as an arg, and the appropriate extra args passed in at the top

JohnGarbutt

I would really like to try an explicit call to set the config mode, rather than changing the existing method.

Either way we need to make sure this doesn't break the other drivers, which is simpler if its just one extra method call.

JohnGarbutt · 2017-10-05T11:17:11Z

networking_generic_switch/devices/netmiko_devices/__init__.py

+                    #     config_mode_cmd='configure private',
+                    #     config_commands=cmd_set)
+                    # FIXME: use the above, commented out version of
+                    # send_config_set once netmiko patch is accepted upstream.


I would try just adding this before the existing send_config_set(), I think that should be OK, because it only activates config mode if its not already enabled:

net_connect.config_mode(config_command='configure private')

technically we would need to copy the save_configuration pattern and add something like start_config_transaction() that is a no op for all drivers except the juniper.

I would have much more hope of getting the above accepted upstream, if that does turn out to work.

If we want to go this route, we would need a new driver method I think, the default could call net_connect.send_config_set and the juniper driver can override that method with the above changes.

Having said that, I still think my preference is trying an explicit start configuration call that only gets made by the juniper.

Just adding net_connect.config_mode(config_command='configure private') would be the ideal solution, I agree, but that's the problem.
As it stands, the send_config_set method in netmiko has a line that reads: output = self.config_mode()
so this will override any value provided (such as 'configure private') and revert to the default for the driver. In the case of juniper, that's ''.
This is why I have had to hack the whole send_config_set method, above.

It's a good point about making this juniper specific though. That was an oversight, I'll fix that now.

JohnGarbutt · 2017-10-05T11:17:29Z

networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py

        connect_mock = mock.MagicMock(SAVE_CONFIGURATION=None)
        connect_mock.__enter__.return_value = connect_mock
        nm_mock.return_value = connect_mock
+


Nit: I guess we can drop this.

autonomouse · 2017-10-05T13:59:28Z

I would too, but as config_mode() is always rerun just prior to write_channel when the content of cmd_set is executed, I don't see a way of doing this, other than what I've done here (at least until that PR to netmiko is accepted)

markgoddard

Thanks Darren, some thoughts inline.

markgoddard · 2017-10-09T13:38:00Z

networking_generic_switch/devices/netmiko_devices/juniper.py

+        if exit_config_mode:
+            output += net_connect.exit_config_mode()
+        output = net_connect._sanitize_output(output)
+        return output


Would this work:

@tenacity.retry(reraise=True, stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_fixed(14)) def send_config_set(self, net_connect, config_commands=None): net_connect.config_mode(config_command='configure private') net_connect.send_config_set(config_commands)

Once set to private config mode, netmiko should not change it to a different config mode.

There's a self.config_mode() call within send_config_set prior to any commands being sent, which presumable would reset the config_command back to it's defaults, which in the case of juniper, would be an empty string. Phil has suggested just monkey patching the config_mode, but it would still need to be called from a monkey-patched send_config_set so we may as well just override that, as I've done here.

Right, but it's a noop if the switch is already in config mode:

def config_mode(self, config_command='', pattern=''): """Enter into config_mode.""" output = '' if not self.check_config_mode(): self.write_channel(self.normalize_cmd(config_command)) output = self.read_until_pattern(pattern=pattern) if not self.check_config_mode(): raise ValueError("Failed to enter configuration mode.") return output

Actually, the monkey patching I'm suggesting would take place entirely in netmiko_devices.juniper.Juniper and amounts to overriding the superclass _get_connection(). Darren has my example code now to take a look at.

markgoddard · 2017-10-09T13:46:00Z

networking_generic_switch/devices/netmiko_devices/juniper.py

    )

+    @tenacity.retry(reraise=True, stop=tenacity.stop_after_attempt(3),
+                    wait=tenacity.wait_fixed(14))


14 seconds seems like quite a long delay. Would it be better to allow more retries and a shorter interval?

Darren and I talked about this. I think he used this time based on my statement as to the average commit duration. I agree with your approach of more retries at a shorter interval.

FYI, I think your retry could just be wrapped around the net_connect.commt() call in Juniper#save_configuration(), when the former raises a ValueError. You might want to check the error message, though, to only retry when it matches the expected error ('error: configuration database locked' or whatever it is).

That sounds like a reasonable approach. If only they'd used a more specific exception we'd be more sure of what had gone wrong.

markgoddard · 2017-10-09T13:49:36Z

networking_generic_switch/devices/netmiko_devices/juniper.py

    )

+    @tenacity.retry(reraise=True, stop=tenacity.stop_after_attempt(3),
+                    wait=tenacity.wait_fixed(14))


In what conditions will this actually perform a retry? I'm not sure there is much error checking at this level, which is in itself an issue. For instance, if a bogus command were to be entered, I'm not sure that netmiko would detect the failure. It should detect failure to commit changes.

markgoddard · 2017-10-26T16:24:42Z

No longer required - superseded by #2

Workaround for netmiko not allowing the config mode command to be set…

c82344d

… (tmp until patch approved upstream)

autonomouse changed the base branch from juniper to stackhpc/pike October 5, 2017 11:02

JohnGarbutt reviewed Oct 5, 2017

View reviewed changes

autonomouse force-pushed the retries_and_configure_private branch from 38ac4be to c82344d Compare October 5, 2017 11:04

autonomouse commented Oct 5, 2017

View reviewed changes

JohnGarbutt reviewed Oct 5, 2017

View reviewed changes

made juniper specific

7e3b658

autonomouse force-pushed the retries_and_configure_private branch from 76825b0 to 7e1569b Compare October 5, 2017 12:46

lint and fixes

54f8727

autonomouse force-pushed the retries_and_configure_private branch from 7e1569b to 54f8727 Compare October 5, 2017 12:47

lint

e71a931

lint

0143af6

autonomouse force-pushed the retries_and_configure_private branch from 9c6874c to 0143af6 Compare October 5, 2017 15:03

markgoddard reviewed Oct 9, 2017

View reviewed changes

markgoddard closed this Oct 26, 2017

Retries and configure private #1

Retries and configure private #1

Uh oh!

Conversation

autonomouse commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnGarbutt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autonomouse commented Oct 5, 2017

Uh oh!

markgoddard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markgoddard commented Oct 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants