MGMT-21993: First pass on cleaning up the log analyzer a bit

[carbonin]

This PR removes some unused functions and gets rid of the old log bundle path which will never be used since we're running against the live service.

More to follow.

Summary by CodeRabbit

• Refactor

  • Consolidated log path handling for improved code maintainability.
  • Removed deprecated internal methods and unused dependencies to streamline the codebase.
  • Simplified analysis signatures with unified path references.

• Tests

  • Updated test suite to align with consolidated logging architecture.

[openshift-ci-robot]

@carbonin: This pull request references MGMT-21993 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

This PR removes some unused functions and gets rid of the old log bundle path which will never be used since we're running against the live service.

More to follow.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

The PR consolidates dual log bundle path constants (NEW_LOG_BUNDLE_PATH, OLD_LOG_BUNDLE_PATH) into a single LOG_BUNDLE_PATH, removing fallback logic throughout the log analyzer module. Additionally removes the get_events_by_host() and get_must_gather() methods, eliminates unused helper functions, and simplifies path resolution across signature files.

Changes

Cohort / File(s)	Summary
Core log analyzer refactoring assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py	Consolidated dual log bundle path constants into single LOG_BUNDLE_PATH; removed get_events_by_host() and get_must_gather() public methods; removed defaultdict import; simplified path handling logic
Main entry point assisted_service_mcp/src/utils/log_analyzer/main.py	Removed print_results() helper function; no changes to analyze_cluster control flow
Signature base and utilities assisted_service_mcp/src/utils/log_analyzer/signatures/base.py	Removed dateutil import and format_time() static method; eliminated ISO time parsing and formatting functionality
Signature analysis files assisted_service_mcp/src/utils/log_analyzer/signatures/advanced_analysis.py, assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py, assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py	Replaced NEW_LOG_BUNDLE_PATH and OLD_LOG_BUNDLE_PATH usage with unified LOG_BUNDLE_PATH across all path lookups; removed legacy fallback logic; simplified path construction for pod configs, kubelet logs, and control-plane directories; removed _search_patterns_in_string helper in error_detection.py
Test files tests/test_advanced_analysis.py	Updated all NEW_LOG_BUNDLE_PATH references to LOG_BUNDLE_PATH in archive map keys and path constructions
Test files tests/test_log_analyzer.py	Removed assertion verifying get_events_by_host() grouping behavior and related test call

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Path consolidation pattern is consistent across multiple files but affects error handling semantics (single-path lookup vs. fallback logic)
• Removed public methods (get_events_by_host, get_must_gather) require verification that no external callers depend on them
• Signature files changes are homogeneous but span five locations; review focus should verify that simplified error handling (returning None on FileNotFoundError) doesn't mask legitimate issues
• Test coverage for removed get_events_by_host() was the only validation method; verify no other tests indirectly depend on it

Possibly related PRs

• MGMT-21946: Crashing container logs #127: Modifies advanced_analysis.py with ContainerCrashAnalysis and archive traversal helpers; directly related to this PR's consolidation of log bundle path handling in the same file.

Suggested labels

lgtm, approved, size/XL

Suggested reviewers

• CrystalChun
• keitwb
• omertuc

Poem

🐰 Two paths have merged into one,
The logger hops lighter in the sun,
Old fallbacks fade, new simplicity shines,
Log bundle dreams flow through single lines!
~CodeRabbit

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 76.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main objective: cleaning up the log analyzer by consolidating log path handling and removing unused functions.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b93ecdc and f1b8099.

📒 Files selected for processing (8)

• assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2 hunks)
• assisted_service_mcp/src/utils/log_analyzer/main.py (0 hunks)
• assisted_service_mcp/src/utils/log_analyzer/signatures/advanced_analysis.py (5 hunks)
• assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (0 hunks)
• assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py (2 hunks)
• assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py (5 hunks)
• tests/test_advanced_analysis.py (13 hunks)
• tests/test_log_analyzer.py (0 hunks)

💤 Files with no reviewable changes (3)

• tests/test_log_analyzer.py
• assisted_service_mcp/src/utils/log_analyzer/main.py
• assisted_service_mcp/src/utils/log_analyzer/signatures/base.py

🧰 Additional context used

🧬 Code graph analysis (4)

assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)

tests/test_advanced_analysis.py (1)

• get (23-26)

tests/test_log_analyzer.py (1)

• get (10-15)

assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py (3)

tests/test_advanced_analysis.py (1)

• get (23-26)

tests/test_log_analyzer.py (1)

• get (10-15)

assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (1)

• create_result (110-126)

assisted_service_mcp/src/utils/log_analyzer/signatures/advanced_analysis.py (3)

tests/test_advanced_analysis.py (1)

• get (23-26)

tests/test_log_analyzer.py (1)

• get (10-15)

assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)

• generate_table (70-74)
• SignatureResult (14-47)

assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py (2)

assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)

• archive_dir_contents (77-89)
• analyze (58-67)

tests/test_log_analyzer.py (1)

• get (10-15)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request

🔇 Additional comments (16)

tests/test_advanced_analysis.py (2)

11-11: LGTM! Path constant import updated correctly.

The import change from NEW_LOG_BUNDLE_PATH to LOG_BUNDLE_PATH aligns with the PR's objective to consolidate path constants.

---

96-97: LGTM! All test path references updated consistently.

All test cases have been updated to use the consolidated LOG_BUNDLE_PATH constant instead of NEW_LOG_BUNDLE_PATH. The changes are mechanical and maintain test coverage.

Also applies to: 117-124, 156-158, 189-191, 221-230, 257-259, 302-306, 326-333, 356-360, 378-379, 446-447, 469-475

assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py (2)

14-14: LGTM! Import updated to use consolidated path constant.

---

86-102: LGTM! Simplified to single-path lookup.

The refactor removes the dual-path fallback logic in favor of a single LOG_BUNDLE_PATH. The error handling correctly returns None when the path is not found, maintaining the same behavior as before.

assisted_service_mcp/src/utils/log_analyzer/signatures/advanced_analysis.py (4)

16-16: LGTM! Import updated correctly.

---

223-267: LGTM! Simplified to single-path lookup.

The refactor consolidates the multi-path iteration to a single LOG_BUNDLE_PATH lookup. The readiness detection logic and table generation remain unchanged, maintaining functional equivalence.

---

278-293: LGTM! Path consolidation implemented correctly.

The method now uses a single path with LOG_BUNDLE_PATH and properly handles FileNotFoundError. The MCD error pattern search logic is preserved.

---

359-395: LGTM! Host directory paths updated consistently.

All path constructions for bootstrap and control-plane directories now use LOG_BUNDLE_PATH. Debug logging is also updated to reference the new constant.

assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (3)

14-15: LGTM! Single log bundle path constant introduced.

The consolidated LOG_BUNDLE_PATH constant simplifies path management across the module.

---

155-173: LGTM! Journal log retrieval simplified.

The method now constructs a single path using LOG_BUNDLE_PATH, removing the dual-path fallback logic.

---

1-195: Removed methods are not referenced elsewhere in the codebase.

Verification complete. Searches for get_events_by_host, get_must_gather, OLD_LOG_BUNDLE_PATH, and NEW_LOG_BUNDLE_PATH across the entire codebase returned zero matches. The methods were correctly removed as unused, and there are no orphaned references to clean up.

assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py (5)

14-14: LGTM! Import updated correctly.

---

77-147: LGTM! Path references updated consistently.

The DuplicateVIP signature now uses LOG_BUNDLE_PATH for all control-plane and bootstrap path constructions. Error handling is preserved.

---

189-216: LGTM! Simplified to single nameserver lookup.

The method now calls _get_nameservers() once with LOG_BUNDLE_PATH instead of iterating over multiple base paths. The overlap detection logic remains unchanged.

---

242-261: LGTM! Single-path lookup implemented correctly.

The DualStackBadRoute signature now uses a single path with LOG_BUNDLE_PATH. Error handling with FileNotFoundError is appropriate.

---

264-281: LGTM! Path consolidation complete.

The DualstackrDNSBug signature now uses LOG_BUNDLE_PATH for the kube-apiserver log path. Error handling is properly implemented.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci-robot]

@carbonin: This pull request references MGMT-21993 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

This PR removes some unused functions and gets rid of the old log bundle path which will never be used since we're running against the live service.

More to follow.

Summary by CodeRabbit

• Refactor

• Consolidated log path handling for improved code maintainability.

• Removed deprecated internal methods and unused dependencies to streamline the codebase.

• Simplified analysis signatures with unified path references.

• Tests

• Updated test suite to align with consolidated logging architecture.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

@carbonin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/eval-test	f1b8099	link	false	/test eval-test

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[CrystalChun]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, CrystalChun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [carbonin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment