Address issue with fact gathering stopping when it encounters a probl…

…em. (#615)

* Address issue with fact gathering stopping when it encounters
a problem. Cleaned up the chassis related fact gathering to
report "UNKNOWN" for model and serial number if it has a problem.

* Updated two unit tests to do with facts gathering via console.
Additional facts are now gathered since we don't stop at the first
exception. The tests had to be updated to account for these additional
facts being gathered.

* Updating test to check for RuntimeError in chassis fact gathering.

* Adding additional tests to improve coverage.

*Fixed an issue where RpcError was not correctly raised
for unknown model or serial number

* Addressing review comments of @vnitinv

* PEP8 cleanup.

lib/jnpr/junos/device.py

@@ -646,17 +646,20 @@ def facts_refresh(self, exception_on_failure=False):
                              facts gathering errors out.
 
         """
+        should_warn = False
         for gather in FACT_LIST:
             try:
                 gather(self, self._facts)
             except:
                 if exception_on_failure:
                     raise
-                warnings.warn('Facts gathering is incomplete. '
-                              'To know the reason call '
-                              '"dev.facts_refresh(exception_on_failure=True)"',
-                              RuntimeWarning)
-                return
+                should_warn = True
+        if should_warn is True:
+            warnings.warn('Facts gathering is incomplete. '
+                          'To know the reason call '
+                          '"dev.facts_refresh(exception_on_failure=True)"',
+                          RuntimeWarning)
+        return
 
     # -----------------------------------------------------------------------
     # OVERLOADS

lib/jnpr/junos/facts/chassis.py

@@ -1,11 +1,14 @@
 from jnpr.junos.exception import ConnectNotMasterError
+from jnpr.junos.exception import RpcError
 
 
 def facts_chassis(junos, facts):
     """
     The following facts are assigned:
-      facts['2RE'] : designates if the device can support two RE, not that it has them
-      facts['RE_hw_mi'] : designates if the device is multi-instance-routing-engine
+      facts['2RE'] : designates if the device can support two RE, not that it
+                     has them
+      facts['RE_hw_mi'] : designates if the device is
+                          multi-instance-routing-engine
       facts['model'] : product model
       facts['serialnumber'] : serial number
 
@@ -15,17 +18,15 @@ def facts_chassis(junos, facts):
         (2) hostname, domain, and fqdn are retrieved from configuration data;
             inherited configs are checked.
     """
-    try:
-        rsp = junos.rpc.get_chassis_inventory()
-        if rsp.tag == 'error':
-            raise RuntimeError()
-    except:
-        # this means that the RPC caused a trap.  this should generally
-        # never happen, but we'll trap it cleanly for now
-        facts['2RE'] = False
-        facts['model'] = ''
-        facts['serialnumber'] = ''
-        return
+    # Set default values.
+    facts['2RE'] = False
+    facts['RE_hw_mi'] = False
+    facts['model'] = 'UNKNOWN'
+    facts['serialnumber'] = 'UNKNOWN'
+
+    rsp = junos.rpc.get_chassis_inventory()
+    if rsp.tag == 'error':
+        raise RuntimeError()
 
     if rsp.tag == 'output':
         # this means that there was an error; due to the
@@ -36,17 +37,15 @@ def facts_chassis(junos, facts):
     if rsp.tag == 'multi-routing-engine-results':
         facts['2RE'] = True
         facts['RE_hw_mi'] = True
-        x_ch = rsp.xpath('.//chassis-inventory')[0].find('chassis')
     else:
         facts['2RE'] = False
-        x_ch = rsp.find('chassis')
 
-    facts['model'] = x_ch.findtext('description')
+    facts['model'] = rsp.findtext('.//chassis[1]/description', 'UNKNOWN')
+    facts['serialnumber'] = (
+        rsp.findtext('.//chassis[1]/serial-number') or
+        rsp.findtext('.//chassis-module[name="Backplane"]/serial-number') or
+        rsp.findtext('.//chassis-module[name="Midplane"]/serial-number',
+                     'UNKNOWN'))
 
-    try:
-        facts['serialnumber'] = x_ch.find('serial-number').text
-    except:
-        # if the toplevel chassis does not have a serial-number, then
-        # check the Backplane chassis-module
-        facts['serialnumber'] = x_ch.xpath(
-            'chassis-module[name="Backplane" or name="Midplane"]/serial-number')[0].text
+    if facts['model'] == 'UNKNOWN' or facts['serialnumber'] == 'UNKNOWN':
+        raise RpcError()

tests/unit/facts/test_chassis.py

@@ -10,6 +10,7 @@
 from jnpr.junos import Device
 from jnpr.junos.facts.chassis import facts_chassis as chassis
 from jnpr.junos.exception import ConnectNotMasterError
+from jnpr.junos.exception import RpcError
 
 from ncclient.manager import Manager, make_device_handler
 from ncclient.transport import SSHSession
@@ -40,9 +41,12 @@ def test_chassis_exception_ConnectNotMasterError(self):
     def test_chassis_exception_RuntimeError(self):
         xmldata = etree.XML('<rpc-reply><error>test</error></rpc-reply>')
         self.dev.rpc.get_chassis_inventory = MagicMock(side_effect=xmldata)
-        chassis(self.dev, self.facts)
-        self.assertFalse(self.facts['2RE'])
-        self.assertEqual(self.facts['model'], self.facts['serialnumber'])
+        self.assertRaises(RuntimeError, chassis, self.dev, self.facts)
+
+    def test_chassis_exception_RpcError(self):
+        xmldata = etree.XML('<rpc-reply><chassis-inventory/></rpc-reply>')
+        self.dev.rpc.get_chassis_inventory = MagicMock(side_effect=xmldata)
+        self.assertRaises(RpcError, chassis, self.dev, self.facts)
 
     def _read_file(self, fname):
         from ncclient.xml_ import NCElement

tests/unit/test_console.py

@@ -163,7 +163,7 @@ def test_console_gather_facts(self, mock_fact_list, mock_rpc):
         from jnpr.junos.facts.session import facts_session
         mock_fact_list.__iter__.return_value = [facts_session]
         self.dev.facts_refresh()
-        self.assertEqual(mock_rpc.call_count, 6)
+        self.assertEqual(mock_rpc.call_count, 8)
 
     @patch('jnpr.junos.console.Console._tty_login')
     @patch('jnpr.junos.console.FACT_LIST')
@@ -172,8 +172,13 @@ def test_console_gather_facts_true(self, mock_fact_list, tty_login):
         mock_fact_list.__iter__.return_value = [facts_session]
         self.dev.gather_facts = True
         self.dev.open()
-        self.assertEqual(self.dev.facts, {'2RE': False, 'serialnumber': '',
-                                          'model': '', 'vc_capable': False,
+        self.assertEqual(self.dev.facts, {'2RE': False,
+                                          'RE_hw_mi': False,
+                                          'ifd_style': 'CLASSIC',
+                                          'serialnumber': 'UNKNOWN',
+                                          'model': 'UNKNOWN',
+                                          'vc_capable': False,
+                                          'switch_style': 'NONE',
                                           'personality': 'UNKNOWN'})
 
     @patch('ncclient.operations.rpc.RPCReply.parse')

tests/unit/test_device.py

@@ -241,6 +241,14 @@ def test_device_facts_error(self, mock_warnings, mock_execute):
             self.dev.facts_refresh()
             self.assertTrue(mock_warnings.warn.called)
 
+    @patch('jnpr.junos.Device.execute')
+    @patch('jnpr.junos.device.warnings')
+    def test_device_facts_error_exception_on_error(self, mock_warnings, mock_execute):
+        with patch('jnpr.junos.utils.fs.FS.cat') as mock_cat:
+            mock_execute.side_effect = self._mock_manager
+            mock_cat.side_effect = IOError('File cant be handled')
+            self.assertRaises(IOError, self.dev.facts_refresh, exception_on_failure=True)
+
     def test_device_hostname(self):
         self.assertEqual(self.dev.hostname, '1.1.1.1')