RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scrip… - #46
RDK-57502 - [RDKE] Migrate Operation Support Log Upload Related Scrip…#46Abhinavpv28 wants to merge 76 commits into
Conversation
…ts To C Implementation (#35) * Created a comprehensive L2 test suite with 6 test modules covering error handling, normal uploads, retry logic, security, resource management, and upload strategies Updated build scripts and CI/CD workflow to compile with L2_TEST_ENABLED flag and execute the new test suite This feature file contains scenarios for normal log uploads, large log file handling, and MD5 checksum verification for the uploadSTBLogs service.
There was a problem hiding this comment.
Pull request overview
This PR migrates operation support log upload scripts from shell to C implementation and adds a comprehensive L2 test suite with 6 test modules covering error handling, normal uploads, retry logic, security, resource management, and upload strategies. The implementation includes conditional compilation for test environments and updates to build scripts and CI/CD workflows.
Key Changes
- Added
L2_TEST_ENABLEDconditional compilation to skip sleep() calls in test environments for faster test execution - Created comprehensive L2 test infrastructure with 6 pytest modules and corresponding BDD feature files
- Updated build scripts (
cov_build.sh) and CI/CD workflow to compile with test flags and execute the new test suite
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 82 comments.
Show a summary per file
| File | Description |
|---|---|
uploadstblogs/src/strategy_reboot.c |
Added conditional compilation guards around sleep() calls to skip delays during L2 testing |
test/run_uploadstblogs_l2.sh |
New test runner script that sets up test environment and executes all 6 L2 test suites |
test/functional-tests/tests/uploadstblogs_helper.py |
New helper module with 260 lines of utility functions for test setup, log analysis, and resource management |
test/functional-tests/tests/test_uploadstblogs_upload_strategies.py |
Test module for upload strategies (on-demand, reboot, DCM scheduled, RBUS integration) |
test/functional-tests/tests/test_uploadstblogs_security.py |
Security test module covering mTLS authentication, SSL validation, and path security |
test/functional-tests/tests/test_uploadstblogs_retry_logic.py |
Test module for network failure handling, retry attempts, and server error responses |
test/functional-tests/tests/test_uploadstblogs_resource_management.py |
Resource management tests covering cleanup, memory management, and concurrent requests |
test/functional-tests/tests/test_uploadstblogs_normal_upload.py |
Test module for normal upload operations and large file handling |
test/functional-tests/tests/test_uploadstblogs_error_handling.py |
Error handling tests for invalid configuration, size limits, and empty logs |
test/functional-tests/features/*.feature |
BDD feature files corresponding to each test module |
cov_build.sh |
Updated to clone feature/upload_L2 branch and compile with L2_TEST_ENABLED flag |
.github/workflows/L2-tests.yml |
Updated CI/CD workflow to execute uploadstblogs L2 tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,86 @@ | |||
| #################################################################################### | |||
| # If not stated otherwise in this file or this component's Licenses | |||
There was a problem hiding this comment.
Incomplete copyright header - line 2 is missing "file the" text. The header should read "If not stated otherwise in this file or this component's Licenses file the" but it currently says "If not stated otherwise in this file or this component's Licenses"
Add the missing text to match the standard copyright header format used in other files.
| # If not stated otherwise in this file or this component's Licenses | |
| # If not stated otherwise in this file or this component's Licenses file the |
| @@ -0,0 +1,48 @@ | |||
| #################################################################################### | |||
| # If not stated otherwise in this file or this component's Licenses | |||
There was a problem hiding this comment.
Incomplete copyright header - line 2 is missing "file the" text. The header should read "If not stated otherwise in this file or this component's Licenses file the" but it currently says "If not stated otherwise in this file or this component's Licenses"
Add the missing text to match the standard copyright header format used in other files.
| # If not stated otherwise in this file or this component's Licenses | |
| # If not stated otherwise in this file or this component's Licenses file the |
| cd telemetry | ||
| cp include/*.h /usr/local/include | ||
| sh build_inside_container.sh | ||
| sh build_inside_container.sh |
There was a problem hiding this comment.
Trailing whitespace at the end of the line. This should be removed for cleaner code formatting.
| cd /usr/common_utilities | ||
| sed -i '/file_upload\.sslverify/s/= 1;/= 0;/' uploadutils/mtls_upload.c | ||
| sed -i 's/\(ret_code = setCommonCurlOpt(curl, s3url, NULL, \)true\()\)/\1false\2/g' uploadutils/uploadUtil.c | ||
| sed -i '/if (auth) {/,/}/s/^/\/\/ /' uploadutils/uploadUtil.c | ||
| cd - | ||
|
|
||
| echo pwd | ||
|
|
||
| # Create log directories |
There was a problem hiding this comment.
These sed commands modify source code in a different repository (common_utilities) during test execution. This is a security and maintainability issue because:
- It modifies source code outside this repository at runtime, making the test behavior unpredictable and dependent on external code structure
- The modifications disable SSL verification and authentication, which are critical security features
- If the external code structure changes, these sed commands could silently fail or produce incorrect modifications
- This makes it difficult to reproduce test results consistently
Consider using proper mocking/stubbing mechanisms or test-specific build configurations instead of runtime source code modifications.
| cd /usr/common_utilities | |
| sed -i '/file_upload\.sslverify/s/= 1;/= 0;/' uploadutils/mtls_upload.c | |
| sed -i 's/\(ret_code = setCommonCurlOpt(curl, s3url, NULL, \)true\()\)/\1false\2/g' uploadutils/uploadUtil.c | |
| sed -i '/if (auth) {/,/}/s/^/\/\/ /' uploadutils/uploadUtil.c | |
| cd - | |
| echo pwd | |
| # Create log directories | |
| # NOTE: Do not modify external common_utilities source here. | |
| # Any SSL/authentication behavior changes for tests should be handled via | |
| # configuration, mocking, or test-specific builds, not in-place sed edits. | |
| echo pwd | |
| # Create log directories | |
| # Create log directories |
| for chunk in iter(lambda: f.read(4096), b""): | ||
| md5_hash.update(chunk) | ||
| return md5_hash.hexdigest() | ||
| except: |
There was a problem hiding this comment.
The bare except clause catches all exceptions including system exits and keyboard interrupts. This makes debugging difficult and can hide serious issues.
Replace with a more specific exception handler like except (OSError, IOError): or except Exception as e: to allow system exceptions to propagate.
| except: | |
| except OSError: |
| import time | ||
| import subprocess as sp | ||
| from uploadstblogs_helper import * | ||
| from helper_functions import * |
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import os | ||
| import time | ||
| import re | ||
| import json |
There was a problem hiding this comment.
Import of 'json' is not used.
…ts To C Implementation (#49) * Simplify some pieces of code over engineered by AI models. * Update run_uploadstblogs_l2.sh for L2 coverage --------- Co-authored-by: Abhinav P V <Abhinav_Valappil@comcast.com>
…ts To C Implementation (#51) * Remove logging of response URLs * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Shibu Kakkoth Vayalambron <shibu.kakkoth@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if (ctx->rrd_flag == 0) { | ||
| // Check PREV_LOG_PATH - critical for upload (matches script behavior) | ||
| if (strlen(ctx->paths.prev_log_path) > 0) { | ||
| if (!dir_exists(ctx->paths.prev_log_path)) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB, "[%s:%d] The Previous Logs folder is missing: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->paths.prev_log_path); | ||
| if (strlen(ctx->prev_log_path) > 0) { | ||
| if (!dir_exists(ctx->prev_log_path)) { | ||
| RDK_LOG(RDK_LOG_ERROR, LOG_UPLOADSTB, "[%s:%d] The Previous Logs folder is missing: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->prev_log_path); | ||
| // Script sends MAINT_LOGUPLOAD_ERROR=5 when PREV_LOG_PATH is missing | ||
| emit_folder_missing_error(); | ||
| all_valid = false; | ||
| } else { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, "[%s:%d] PREV_LOG_PATH exists: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->paths.prev_log_path); | ||
| emit_folder_missing_error(); | ||
| all_valid = false; | ||
| } else { | ||
| RDK_LOG(RDK_LOG_DEBUG, LOG_UPLOADSTB, "[%s:%d] PREV_LOG_PATH exists: %s\n", | ||
| __FUNCTION__, __LINE__, ctx->prev_log_path); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The structure flattening from nested structs (ctx.paths.log_path) to flat fields (ctx.log_path) is inconsistently applied. In validation.c line 85, there's a condition if (ctx->rrd_flag == 0) wrapping the PREV_LOG_PATH validation. This condition was not present in the original code and changes the validation logic. The PREV_LOG_PATH validation should run unconditionally as it did before, not only when rrd_flag is 0.
| result = run_uploadstblogs() | ||
|
|
||
| # Check for telemetry markers | ||
| telemetry_logs = grep_uploadstb_logs_regex(r"telemetry|marker|warning") |
There was a problem hiding this comment.
Variable telemetry_logs is not used.
| telemetry_logs = grep_uploadstb_logs_regex(r"telemetry|marker|warning") | |
| telemetry_logs = grep_uploadstb_logs_regex(r"telemetry|marker|warning") | |
| assert telemetry_logs, "Expected telemetry markers in uploadSTB logs" |
| result = run_uploadstblogs() | ||
|
|
||
| # Check for informative message | ||
| logs = grep_uploadstb_logs_regex(r"no.*file|empty|nothing to upload") |
There was a problem hiding this comment.
Variable logs is not used.
|
|
||
| # Should not attempt upload | ||
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | ||
| # Might not see upload attempts |
There was a problem hiding this comment.
Variable upload_logs is not used.
| # Might not see upload attempts | |
| # Verify that no upload-related log entries are present | |
| assert len(upload_logs) == 0, "Should not attempt upload when no log files are available" |
…ts To C Implementation (#54) * Update cov_build.sh to enable the rdkcertselector feature in the common_utilities build configuration to support the migration of operation support log upload scripts to C implementation. The change is part of resolving L2 test failures.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 26 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check for configuration error detection | ||
| error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid") | ||
| # Should handle gracefully | ||
| assert result.returncode in [0, 1], "Should detect corrupted config" |
There was a problem hiding this comment.
Variable error_logs is not used.
| assert result.returncode in [0, 1], "Should detect corrupted config" | |
| assert result.returncode in [0, 1], "Should detect corrupted config" | |
| assert len(error_logs) > 0 or result.returncode != 0, "Configuration errors should be logged" |
| result = run_uploadstblogs() | ||
|
|
||
| # Check for size-related logs | ||
| logs = grep_uploadstb_logs_regex(r"size|large|limit|truncate") |
There was a problem hiding this comment.
Variable logs is not used.
| logs = grep_uploadstb_logs_regex(r"size|large|limit|truncate") | |
| grep_uploadstb_logs_regex(r"size|large|limit|truncate") |
|
|
||
| # Should not attempt upload | ||
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | ||
| # Might not see upload attempts |
There was a problem hiding this comment.
Variable upload_logs is not used.
| # Might not see upload attempts | |
| # Ensure no upload-related entries are present in the logs | |
| assert len(upload_logs) == 0, "No upload logs should be present when there are no files" | |
| # Process should still handle the no-files case gracefully |
| assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" | ||
|
|
||
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | ||
| # Telemetry should be attempted | ||
| assert len(archive_logs) > 0, "Upload Process should complete and succeed" |
There was a problem hiding this comment.
Variable upload_logs is not used.
| assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" | |
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | |
| # Telemetry should be attempted | |
| assert len(archive_logs) > 0, "Upload Process should complete and succeed" | |
| assert len(archive_logs) > 0, ( | |
| f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" | |
| ) | |
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | |
| # Telemetry should be attempted | |
| assert len(upload_logs) > 0, ( | |
| f"Upload process should complete and succeed. " | |
| f"Found {len(upload_logs)} upload-related logs: {upload_logs}" | |
| ) |
| # Verify files were processed | ||
|
|
||
| # Check for compression/archive activity | ||
| compression_logs = grep_uploadstb_logs_regex(r"compress|archive|tgz") |
There was a problem hiding this comment.
Variable compression_logs is not used.
| compression_logs = grep_uploadstb_logs_regex(r"compress|archive|tgz") |
| # Check for configuration loading | ||
| config_logs = grep_uploadstb_logs_regex(r"load.*TR-181|load.*param|endpoint|RFC") | ||
| # Should attempt to load config | ||
| assert result.returncode in [0, 1], "Should load RBUS configuration" |
There was a problem hiding this comment.
Variable config_logs is not used.
| assert result.returncode in [0, 1], "Should load RBUS configuration" | |
| assert result.returncode in [0, 1], "Should load RBUS configuration" | |
| assert config_logs, "RBUS configuration logs should be present" |
| try: | ||
| subprocess.run(f"echo '' > {UPLOADSTB_LOG}", shell=True) | ||
| return True | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| if os.path.exists(LOCK_FILE): | ||
| os.remove(LOCK_FILE) | ||
| return True | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| for chunk in iter(lambda: f.read(4096), b""): | ||
| md5_hash.update(chunk) | ||
| return md5_hash.hexdigest() | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| """Get file size in bytes""" | ||
| try: | ||
| return os.path.getsize(filepath) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 63 changed files in this pull request and generated 31 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = run_uploadstblogs() | ||
|
|
||
| # Check for size limit detection | ||
| size_logs = grep_uploadstb_logs_regex(r"size.*limit|too large|exceed") |
There was a problem hiding this comment.
Variable size_logs is not used.
| size_logs = grep_uploadstb_logs_regex(r"size.*limit|too large|exceed") | |
| size_logs = grep_uploadstb_logs_regex(r"size.*limit|too large|exceed") | |
| assert size_logs, "Expected size-related log entries for oversized file" |
| result = run_uploadstblogs() | ||
|
|
||
| # Check for no files detection | ||
| no_files_logs = grep_uploadstb_logs_regex(r"no.*file|empty|not found|no logs") |
There was a problem hiding this comment.
Variable no_files_logs is not used.
| telemetry_logs = grep_uploadstb_logs_regex(r"telemetry|marker|event") | ||
| # Process should complete | ||
| assert result.returncode in [0, 1], "Should handle empty logs with telemetry" | ||
|
|
There was a problem hiding this comment.
Variable telemetry_logs is not used.
| # Telemetry entries should be present | |
| assert len(telemetry_logs) > 0, "Expected telemetry markers for empty logs" |
|
|
||
| create_test_log_files(count=2) | ||
|
|
||
| result = run_uploadstblogs() |
There was a problem hiding this comment.
Variable result is not used.
| result = run_uploadstblogs() | |
| run_uploadstblogs() |
|
|
||
| create_test_log_files(count=1) | ||
|
|
||
| result = run_uploadstblogs() |
There was a problem hiding this comment.
Variable result is not used.
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import time |
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import time |
| import os | ||
| import time | ||
| import re | ||
| import json |
There was a problem hiding this comment.
Import of 'json' is not used.
| import json |
| for chunk in iter(lambda: f.read(4096), b""): | ||
| md5_hash.update(chunk) | ||
| return md5_hash.hexdigest() | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| except: | |
| except Exception: |
| """Get file size in bytes""" | ||
| try: | ||
| return os.path.getsize(filepath) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| except: | |
| except OSError: |
DCM Agent 2.0.1 release
* Create uploadlogsnow.h --------- Co-authored-by: nhanasi <navihansi@gmail.com>
* Update L2-tests.yml * L2 automatics upload Error --------- Co-authored-by: Abhinav P V <Abhinav_Valappil@comcast.com>
…161) * Update dcm_parseconf.c * Update log message for backup_logs sentinel detection * Update backup_engine.c * Remove old copyright and documentation comments
* Update archive_manager.h * Update archive_manager.c * Update strategies.c * Update archive_manager.c * Update archive_manager.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Fix formatting of comment in strategies.c * Update archive_manager_gtest.cpp * Update strategies_gtest.cpp
…from RDK analytics portal (#169) * Update strategies.c * Update strategies.c * Remove unnecessary blank line in strategies.c
…ed to logupload server is not available (#168) * Update archive_manager.c * Update backup_engine.c * Update backup_engine_gtest.cpp * Update backup_engine_gtest.cpp * Update Makefile.am * Update Makefile.am * Update Makefile.am * Update backup_engine_gtest.cpp * Update backup_logs_gtest.cpp * Update Makefile.am * Update backup_engine_gtest.cpp * Update backup_logs_gtest.cpp * Update backup_logs_gtest.cpp * Update backup_engine_gtest.cpp * Update backup_logs_gtest.cpp * Update Makefile.am * Update backup_engine_gtest.cpp * Update Makefile.am * Update Makefile.am * Update Makefile.am * Update Makefile.am * Update backup_engine_gtest.cpp * Update backup_engine.c * Clean up mock functions and tests for backup logs Removed unused mock open functions and related tests for backup_and_recover_logs. * Remove -U_FORTIFY_SOURCE from backup_engine_gtest_CFLAGS
…ts To C Implementation (#35)
This feature file contains scenarios for normal log uploads, large log file handling, and MD5 checksum verification for the uploadSTBLogs service.