feat: add get_config support to device-discovery#267
feat: add get_config support to device-discovery#267leoparente wants to merge 2 commits intodevelopfrom
Conversation
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mfiedorowicz
left a comment
mfiedorowicz
left a comment
There was a problem hiding this comment.
Argus Code Review Summary
- 🟡 Medium: 6
- 🔵 Low: 6
🔍 Automated review by Argus on behalf of @mfiedorowicz
| chunk_num = 1 | ||
| size_bytes = estimate_message_size(entities_list) | ||
|
|
||
| if size_bytes > (3.0 * 1024 * 1024): # 3MB threshold |
There was a problem hiding this comment.
🟡 Medium
The 3MB threshold is hardcoded as a magic number. More importantly, estimate_message_size and create_message_chunks may use different internal thresholds for chunking, leading to inconsistency. Consider using a named constant and verifying alignment with the SDK's chunking logic.
| if size_bytes > (3.0 * 1024 * 1024): # 3MB threshold | |
| MAX_MESSAGE_SIZE_BYTES = 3 * 1024 * 1024 # 3MB threshold | |
| if size_bytes > MAX_MESSAGE_SIZE_BYTES: |
| f"ERROR ingestion failed for {hostname} chunk {i}/{chunk_num}: " | ||
| f"{response.errors}" | ||
| ) | ||
| return # Stop on first error |
There was a problem hiding this comment.
🟡 Medium
On chunk ingestion error, the method returns None silently. The caller has no way to know ingestion failed — partial data was ingested (some chunks succeeded) but no error is raised or returned. This is inconsistent with the non-chunked path which also silently logs. Consider raising an exception or returning a status so callers can handle failures.
| logger.info(f"Hostname {hostname}: Successful ingestion") | ||
|
|
||
| # Convert to list for size estimation and chunking | ||
| entities_list = list(translated_entities) |
There was a problem hiding this comment.
🔵 Low
Calling list(translated_entities) materializes the entire generator into memory. If translate_data returns a very large dataset (which is likely given the need for chunking at 3MB+), this doubles memory usage — once for the list, then again for the chunks. Consider whether the SDK's create_message_chunks could accept an iterator, or whether size estimation could be done differently.
| @@ -119,15 +125,50 @@ def ingest(self, metadata: dict[str, Any] | None, data: dict): | |||
| with self._lock: | |||
There was a problem hiding this comment.
🔵 Low
The entire ingest operation (including network calls to self.diode_client.ingest for potentially multiple chunks) is performed while holding self._lock. This means all other operations (including init_client and concurrent ingest calls) are blocked for the duration of potentially slow network I/O. Consider whether finer-grained locking is appropriate — e.g., only lock around shared state access, not the network calls.
| running = running.encode("utf-8") | ||
|
|
||
| # Skip if no actual config data present | ||
| if not any([startup, running]): |
There was a problem hiding this comment.
🟡 Medium
Using any() with a list literal is unnecessary and slightly less efficient. any([startup, running]) creates a list before evaluating; use any((startup, running)) or simply startup or running instead.
| if not any([startup, running]): | |
| if not (startup or running): |
| result = translate_device_config(config_info, options) | ||
|
|
||
| # Should return None since pb.DeviceConfig doesn't exist yet | ||
| if not _has_device_config: |
There was a problem hiding this comment.
🟡 Medium
Test test_translate_device_config_returns_none_when_sdk_unavailable only asserts when _has_device_config is False. If the SDK becomes available, this test silently passes without asserting anything, making it a no-op. It should assert the positive case too (result is not None when SDK is available).
| if not _has_device_config: | |
| if not _has_device_config: | |
| assert result is None | |
| else: | |
| assert result is not None |
| if _has_device_config and result is not None: | ||
| # Would check that result contains bytes, not strings | ||
| # This will be testable once pb.DeviceConfig exists | ||
| assert True # Placeholder for future validation |
There was a problem hiding this comment.
🟡 Medium
Placeholder assert True makes this test a no-op when the SDK is available. The test claims to verify string-to-bytes conversion but never actually checks it. Either implement the actual assertion or skip the test with pytest.skip() to signal it's not yet functional.
| assert True # Placeholder for future validation | |
| if _has_device_config and result is not None: | |
| # Verify configs were converted to bytes | |
| assert isinstance(result.running, bytes) | |
| assert isinstance(result.startup, bytes) | |
| else: | |
| pytest.skip("pb.DeviceConfig not yet available in SDK") |
|
|
||
| if _has_device_config and result is not None: | ||
| # Bytes should pass through without conversion | ||
| assert True # Placeholder for future validation |
There was a problem hiding this comment.
🟡 Medium
Same assert True placeholder issue as the string-to-bytes test. This test provides no actual validation when the SDK is available.
| assert True # Placeholder for future validation | |
| if _has_device_config and result is not None: | |
| assert isinstance(result.running, bytes) | |
| assert isinstance(result.startup, bytes) | |
| else: | |
| pytest.skip("pb.DeviceConfig not yet available in SDK") |
| assert result is None | ||
|
|
||
|
|
||
| def test_translate_device_config_with_none_config(): |
There was a problem hiding this comment.
🔵 Low
Test name says 'with_none_config' but the docstring says 'with None config', yet the test actually passes an empty dict {}, not None. This is identical to test_translate_device_config_with_empty_config. Consider actually passing None to test that code path.
| def test_translate_device_config_with_none_config(): | |
| # Should handle None gracefully | |
| result = translate_device_config(None, options) | |
| assert result is None |
|
|
||
| entities = list(translate_data(data)) | ||
|
|
||
| # Should have at least device entity |
There was a problem hiding this comment.
🔵 Low
The assertion len(entities) > 0 is weak. Since you know the data has a device and no interfaces/IPs, you should assert the exact expected count (1) for a more precise test.
| # Should have at least device entity | |
| assert len(entities) == 1 |
2 participants
This pull request introduces support for translating and ingesting device configuration data (such as startup, running, and candidate configs) from NAPALM into the Diode SDK, while maintaining compatibility with environments where the
DeviceConfigprotobuf message is not yet available. The changes are structured to gracefully handle the absence of this feature in the SDK and to ensure robust error handling during data collection.Device configuration ingestion and translation:
runner.pyto collect device configuration data with error handling, ensuring that failures to retrieve config do not interrupt the data collection process.DeviceConfigwrapper and related translation functions intranslate.pyto convert NAPALM config data into the Diode SDKDeviceConfigprotobuf, including safe handling when the SDK does not yet support this message. [1] [2]translate_deviceandtranslate_datafunctions to pass configuration data through the translation pipeline and attach it to the resulting device entity when supported. [1] [2] [3] [4]Type hinting and imports: