Validate and cast discovery_interval to a number (#5887)

* Validate and parse `discovery_interval` as an integer

* Switch to float, fix unit test

* Pass args to Thread

snmp/datadog_checks/snmp/snmp.py

@@ -129,15 +129,10 @@ def _get_instance_name(self, instance):
         else:
             return None
 
-    def discover_instances(self):
-        # type: () -> None
+    def discover_instances(self, interval):
+        # type: (float) -> None
         config = self._config
 
-        if config.ip_network is None:
-            raise RuntimeError("Expected config.ip_network to be set to start discovery")
-
-        discovery_interval = config.instance.get('discovery_interval', 3600)
-
         while self._running:
             start_time = time.time()
             for host in config.network_hosts():
@@ -171,8 +166,8 @@ def discover_instances(self):
             write_persistent_cache(self.check_id, json.dumps(list(config.discovered_instances)))
 
             time_elapsed = time.time() - start_time
-            if discovery_interval - time_elapsed > 0:
-                time.sleep(discovery_interval - time_elapsed)
+            if interval - time_elapsed > 0:
+                time.sleep(interval - time_elapsed)
 
     def raise_on_error_indication(self, error_indication, ip_address):
         # type: (Any, Optional[str]) -> None
@@ -356,7 +351,14 @@ def _start_discovery(self):
                 host_config = self._build_config(instance)
                 self._config.discovered_instances[host] = host_config
 
-        self._thread = threading.Thread(target=self.discover_instances, name=self.name)
+        raw_discovery_interval = self._config.instance.get('discovery_interval', 3600)
+        try:
+            discovery_interval = float(raw_discovery_interval)
+        except (ValueError, TypeError):
+            message = 'discovery_interval could not be parsed as a number: {!r}'.format(raw_discovery_interval)
+            raise ConfigurationError(message)
+
+        self._thread = threading.Thread(target=self.discover_instances, args=(discovery_interval,), name=self.name)
         self._thread.daemon = True
         self._thread.start()
         self._executor = futures.ThreadPoolExecutor(max_workers=self._config.workers)

snmp/tests/test_unit.py

@@ -215,6 +215,20 @@ def test_removing_host():
     assert warnings == [msg, msg, msg]
 
 
+def test_invalid_discovery_interval():
+    instance = common.generate_instance_config(common.SUPPORTED_METRIC_TYPES)
+
+    # Trigger autodiscovery.
+    instance.pop('ip_address')
+    instance['network_address'] = '192.168.0.0/24'
+
+    instance['discovery_interval'] = 'not_parsable_as_a_float'
+
+    check = SnmpCheck('snmp', {}, [instance])
+    with pytest.raises(ConfigurationError):
+        check.check(instance)
+
+
 @mock.patch("datadog_checks.snmp.snmp.read_persistent_cache")
 def test_cache_discovered_host(read_mock):
     instance = common.generate_instance_config(common.SUPPORTED_METRIC_TYPES)
@@ -346,7 +360,6 @@ def test_discovery_tags():
     instance.pop('ip_address')
 
     instance['network_address'] = '192.168.0.0/29'
-    instance['discovery_interval'] = 0
     instance['tags'] = ['test:check']
 
     check = SnmpCheck('snmp', {}, [instance])
@@ -361,7 +374,7 @@ def mock_fetch(cfg):
 
     check.fetch_sysobject_oid = mock_fetch
 
-    check.discover_instances()
+    check.discover_instances(interval=0)
 
     config = check._config.discovered_instances['192.168.0.2']
     assert set(config.tags) == {'snmp_device:192.168.0.2', 'test:check'}