Skip to content

Retries and configure private #1

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

Closed
wants to merge 5 commits into from
Closed

Retries and configure private #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c82344d
Workaround for netmiko not allowing the config mode command to be set…
Oct 5, 2017
7e3b658
made juniper specific
Oct 5, 2017
54f8727
lint and fixes
Oct 5, 2017
e71a931
lint
Oct 5, 2017
0143af6
lint
Oct 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ develop-eggs
.venv
.coverage
cover
.stestr

# Others
.*.swp
Expand Down
7 changes: 5 additions & 2 deletions networking_generic_switch/devices/netmiko_devices/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ def _create_connection():
with net_connect:
yield net_connect

def send_config_set(self, net_connect, config_commands):
return net_connect.send_config_set(config_commands=config_commands)

def send_commands_to_device(self, cmd_set):
if not cmd_set:
LOG.debug("Nothing to execute")
Expand All @@ -136,8 +139,8 @@ def send_commands_to_device(self, cmd_set):
with ngs_lock.PoolLock(self.locker, **self.lock_kwargs):
with self._get_connection() as net_connect:
net_connect.enable()
output = net_connect.send_config_set(
config_commands=cmd_set)
output = self.send_config_set(
net_connect, config_commands=cmd_set)
# NOTE (vsaienko) always save configuration
# when configuration is applied successfully.
self.save_configuration(net_connect)
Expand Down
43 changes: 43 additions & 0 deletions networking_generic_switch/devices/netmiko_devices/juniper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
import time

import tenacity

from netmiko.py23_compat import string_types

from networking_generic_switch.devices import netmiko_devices

Expand Down Expand Up @@ -49,6 +54,44 @@ class Juniper(netmiko_devices.NetmikoSwitch):
'vlan members {segmentation_id}',
)

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

@markgoddard markgoddard Oct 9, 2017

Choose a reason for hiding this comment

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

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

Copy link

@pheller pheller Oct 9, 2017

Choose a reason for hiding this comment

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

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.

Copy link

@pheller pheller Oct 9, 2017

Choose a reason for hiding this comment

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

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).

Copy link

@markgoddard markgoddard Oct 9, 2017

Choose a reason for hiding this comment

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

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.

Copy link

@markgoddard markgoddard Oct 9, 2017

Choose a reason for hiding this comment

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

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.

def send_config_set(self, net_connect, config_commands=None,
exit_config_mode=True, delay_factor=1, max_loops=150,
strip_prompt=False, strip_command=False,
config_mode_cmd='configure private'):
"""Temporarily overriding the send_config_set method from netmiko.

Temporarily overriding the send_config_set method from netmiko, until
upstream patch is accepted:

https://github.com/ktbyers/netmiko/pull/593

"""

delay_factor = net_connect.select_delay_factor(delay_factor)
if config_commands is None:
return ''
elif isinstance(config_commands, string_types):
config_commands = (config_commands,)

if not hasattr(config_commands, '__iter__'):
raise ValueError("Invalid argument passed into send_config_set")

# Send config commands
output = net_connect.config_mode(config_mode_cmd)
for cmd in config_commands:
net_connect.write_channel(net_connect.normalize_cmd(cmd))
time.sleep(delay_factor * .5)

# Gather output
output += net_connect._read_channel_timing(
delay_factor=delay_factor, max_loops=max_loops)
if exit_config_mode:
output += net_connect.exit_config_mode()
output = net_connect._sanitize_output(output)
return output
Copy link

@markgoddard markgoddard Oct 9, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Author

@autonomouse autonomouse Oct 9, 2017

Choose a reason for hiding this comment

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

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.

Copy link

@markgoddard markgoddard Oct 9, 2017

Choose a reason for hiding this comment

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

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

Copy link

@pheller pheller Oct 9, 2017

Choose a reason for hiding this comment

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

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.


def save_configuration(self, net_connect):
"""Save the device's configuration.

Expand Down
3 changes: 2 additions & 1 deletion networking_generic_switch/tests/unit/netmiko/test_juniper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ def test_add_network_with_trunk_ports(self, mock_exec):
'NetmikoSwitch.send_commands_to_device')
def test_del_network(self, mock_exec):
self.switch.del_network(33, '0ae071f55be943e480eae41fefe85b21')
mock_exec.assert_called_with(['delete vlans 0ae071f55be943e480eae41fefe85b21'])
mock_exec.assert_called_with([
'delete vlans 0ae071f55be943e480eae41fefe85b21'])

@mock.patch('networking_generic_switch.devices.netmiko_devices.'
'NetmikoSwitch.send_commands_to_device')
Expand Down