napalm-automation / napalm-ios

Apache License 2.0
32 stars 40 forks source link

move auto detetec to merge and replace only #171

Closed itdependsnetworks closed 7 years ago

itdependsnetworks commented 7 years ago
Addresses #148 I believe the only two places the file system is actually needed is on merge and replace.
coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.2%) to 70.52% when pulling 6ad5fed4eead4c92ccee02e3e69c63ef330a1658 on itdependsnetworks:auto_detect into 42f1a7ff6b6ea88f96f8f91bc6ce68ad25a5fd96 on napalm-automation:develop.

ktbyers commented 7 years ago

I think we should just convert this to a Python setter/getter format which @dbarrosop mentioned previously. Basically, whenever anyone accesses this attribute, it will go and do the autodetect if it is not currently sent.

This will prevent the current solution from breaking if we expand the use of this attribute.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 71.263% when pulling d523fca8922ca4196d53ea1a94acbe06dc9ddc7d on itdependsnetworks:auto_detect into 42f1a7ff6b6ea88f96f8f91bc6ce68ad25a5fd96 on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 71.263% when pulling d523fca8922ca4196d53ea1a94acbe06dc9ddc7d on itdependsnetworks:auto_detect into 42f1a7ff6b6ea88f96f8f91bc6ce68ad25a5fd96 on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 71.263% when pulling d523fca8922ca4196d53ea1a94acbe06dc9ddc7d on itdependsnetworks:auto_detect into 42f1a7ff6b6ea88f96f8f91bc6ce68ad25a5fd96 on napalm-automation:develop.

ktbyers commented 7 years ago

I think this will fix the testing error:

diff --git a/napalm_ios/ios.py b/napalm_ios/ios.py
index c72ef66..e184c11 100644
--- a/napalm_ios/ios.py
+++ b/napalm_ios/ios.py
@@ -139,9 +139,10 @@ class IOSDriver(NetworkDriver):
     def _discover_file_system(self):
         try:
             return self.device._autodetect_fs()
-        except Exception:
-            raise Exception("Netmiko _autodetect_fs failed to workaround specify "
-                                 "dest_file_system in optional_args.")
+        except Exception as e:
+            e.message = "Netmiko _autodetect_fs failed to workaround specify " \
+                        "dest_file_system in optional_args."
+            raise

     def is_alive(self):
@@ -168,13 +169,14 @@ class IOSDriver(NetworkDriver):
         """Returns a flag with the state of the SSH connection."""
         null = chr(0)
         try:
-            # Try sending ASCII null byte to maintain
-            #   the connection alive
-            self.device.send_command(null)
+            if self.device is None:
+                return {'is_alive': False}
+            else:
+                # Try sending ASCII null byte to maintain the connection alive
+                self.device.send_command(null)
         except (socket.error, EOFError):
-            # If unable to send, we can tell for sure
-            #   that the connection is unusable,
-            #   hence return False.
+            # If unable to send, we can tell for sure that the connection is unusable,
+            # hence return False.
             return {
                 'is_alive': False
             }
ktbyers commented 7 years ago

The is_alive fix was because I saw this when I ran the tests:

test/unit/test_getters.py::TestGetter::test_method_signatures <- ../napalm-base/napalm_base/test/getters.py Exception ignored in: <bound method IOSDriver.__del__ of <napalm_ios.ios.IOSDriver object at 0xb5ff8dcc>>
Traceback (most recent call last):
  File "/home/gituser/NAPALM/napalm/napalm-base/napalm_base/base.py", line 69, in __del__
    if self.is_alive()["is_alive"]:
  File "/home/gituser/NAPALM/napalm/napalm-ios/napalm_ios/ios.py", line 184, in is_alive
    'is_alive': self.device.remote_conn.transport.is_active()
AttributeError: 'NoneType' object has no attribute 'remote_conn'

and a similar error for send_command

ktbyers commented 7 years ago

The is_alive method should look like this:

    def is_alive(self):
        """Returns a flag with the state of the SSH connection."""
        null = chr(0)
        try: 
            if self.device is None:
                return {'is_alive': False}
            else:
                # Try sending ASCII null byte to maintain the connection alive
                self.device.send_command(null)
        except (socket.error, EOFError):
            # If unable to send, we can tell for sure that the connection is unusable,
            # hence return False.
            return {
                'is_alive': False
            }    
        return {
            'is_alive': self.device.remote_conn.transport.is_active()
        } 

If you want me to submit this into a separate PR that is fine also.

Basically make sure it doesn't run this if self.device hasn't even been initialized.

ktbyers commented 7 years ago

What I posted above doesn't work. I will correct.

ktbyers commented 7 years ago

Made an updated PR (built upon @itdependsnetworks work here):

https://github.com/napalm-automation/napalm-ios/pull/176

Closing this one.