More log analyzer cleanup#155
More log analyzer cleanup#155openshift-merge-bot[bot] merged 7 commits intoopenshift-assisted:masterfrom
Conversation
openshift-ci
Bot
added
the
approved
WalkthroughThis pull request restructures the log analyzer metadata layout from nested cluster-scoped fields to root-level fields, reorganizes signature classes from consolidated modules into individual files (approximately 20 new signature modules), removes deprecated signatures and modules, and adds utility helpers for operator status parsing and hostname extraction. The changes consolidate log analysis patterns and improve modularity. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as LogAnalyzer User
participant LogAnalyzer as LogAnalyzer
participant MetadataRoot as Metadata (Root Level)
participant HostIter as all_host_journal_logs()
participant Sig as Signature
Client->>LogAnalyzer: metadata()
LogAnalyzer->>MetadataRoot: Read raw_metadata (root level)
LogAnalyzer->>LogAnalyzer: _clean_metadata_json()
MetadataRoot->>LogAnalyzer: md["hosts"], md["install_started_at"]
LogAnalyzer-->>Client: Cleaned metadata
Client->>LogAnalyzer: all_host_journal_logs()
LogAnalyzer->>HostIter: Iterate hosts from md["hosts"]
loop For each host
HostIter->>LogAnalyzer: get_host_log_file(host_id, "journal.logs")
alt File exists
LogAnalyzer-->>HostIter: (host, journal_logs)
else FileNotFoundError
HostIter->>HostIter: Skip host
end
end
HostIter-->>Client: Iterator[Tuple[host, logs]]
Client->>Sig: analyze(log_analyzer)
Sig->>LogAnalyzer: get_controller_logs() or hosts metadata
LogAnalyzer-->>Sig: Log content or host data
Sig->>Sig: Pattern match / validate
Sig-->>Client: SignatureResult or None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
openshift-ci
Bot
added
the
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
assisted_service_mcp/src/utils/log_analyzer/signatures/notes (1)
1-12: Notes file is fine; consider tidying format for clarityThis looks like a pure notes/documentation file. If it’s meant to be human-only (not Python), you might consider:
- Dropping the leading
-so lines are just# ..., or- Renaming to
notes.md/NOTES.txtto avoid any impression that it’s executable.Purely cosmetic; no functional issues here.
tests/utils/test_auth.py (1)
3-40: Autouse reload fixture gives clean auth state per testThe
reset_auth_modulefixture combined with per-test imports is a straightforward way to avoid cross-test leakage from module-level state or patches. The extra reload after each test is defensive but harmless; if test runtime ever becomes a concern, you could drop the post-yield reload and rely on the pre-yield reload alone.assisted_service_mcp/src/utils/log_analyzer/signatures/libvirt_reboot_flag_signature.py (1)
19-51: Consider hardening inventory parsing and clarifying the host thresholdThe core matching logic looks correct for the described MGMT-2840 scenario, but there are a couple of robustness points you might want to tighten:
inventory = json.loads(host.get("inventory", "{}"))will raise ifinventoryis present but malformed JSON. That would abort the entire signature instead of just skipping a bad host. Wrapping this in a smalltry/except json.JSONDecodeError(and continuing on error) would keep the analysis resilient to partial data issues.- The final condition
if hosts and len(hosts) + 1 == len(cluster.get("hosts", [])):encodes “all but one host match” implicitly. Given this is quite specific, a brief comment or helper (e.g.,all_but_one_hosts_match = ...) would make the intention clearer for future maintainers.- Since
generate_tableis defined inbase.py, please confirm it’s exposed on the signature base class (soself.generate_tableis valid) rather than only as a free function.None of these are blockers, but addressing them would make the signature more robust and self-explanatory.
assisted_service_mcp/src/utils/log_analyzer/signatures/error_creating_read_write_layer.py (1)
16-60: ErrorCreatingReadWriteLayer signature mirrors prior behavior and is defensively codedThe signature logic matches the described intent: it traverses must-gather namespaces, safely skips missing pods.yaml files, and only emits a result when it finds pods with
CreateContainerErrorand the specific “error creating read-write layer with ID” message. The broadexcept Exceptionaround YAML parsing is acceptable here to avoid a single bad namespace breaking the analysis; if you ever need deeper troubleshooting, adding a debug-level log inside thatexceptwould be a small quality-of-life improvement.assisted_service_mcp/src/utils/log_analyzer/signatures/sno_hostname_has_etcd.py (1)
14-38: Logic looks correct; consider case‑insensitive match for robustnessThe gating on
cluster_is_sno(), single host, and use ofget_hostnameis appropriate and the error handling pattern is consistent with other signatures. As a small robustness improvement, you could normalize the hostname before checking:hostname = get_hostname(cluster["hosts"][0]) if "etcd" in hostname.lower(): ...This would avoid missing any unexpected casing in hostnames while preserving current behavior.
assisted_service_mcp/src/utils/log_analyzer/signatures/duplicate_vip.py (1)
75-86: Clarify duplicate VIP message when bootstrap is involved
dup_msgscurrently say “Found duplicate VIP {vip} in control-plane hosts ...”, butnodescan include"bootstrap"as well as control-plane nodes. To avoid confusing output like “in control-plane hosts bootstrap and 10.0.0.1”, consider slightly more generic wording, e.g.:f"Found duplicate VIP {vip} in hosts {' and '.join(sorted(nodes))}"so the message accurately reflects both control-plane and bootstrap participants.
assisted_service_mcp/src/utils/log_analyzer/signatures/missing_mc.py (1)
14-39: MissingMC behavior looks correct; optionally log decode failuresThe SNO guard, path selection, and regex for
rendered-master-<hash> ... not foundall look correct for detecting this condition, and returning None on missing files is reasonable.If you want easier debugging when archives are malformed, you could optionally log when the decode step fails instead of silently returning None in the
except Exception:block so operators can distinguish “no issue” from “could not parse machineconfigpools/master.yaml”.assisted_service_mcp/src/utils/log_analyzer/signatures/release_pull_error_signature.py (1)
15-34: ReleasePullErrorSignature aligns well withall_host_journal_logs; minor micro‑optimization possibleIterating
all_host_journal_logs()and emitting one line per affected host usingget_hostname(host)is a good fit for this signature, and the resultingSignatureResultshape is consistent with other error signatures.Since you only need to know whether a host’s journal contains at least one failure, you could slightly simplify the condition to use
searchinstead offindall:if self.ERROR_PATTERN.search(journal_logs): ...Functionally it’s equivalent, just a bit cheaper on large logs.
assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py (1)
21-38: ApiExpiredCertificateSignature implementation looks solid; consider minor robustness tweaksThe overall flow (single log read,
FileNotFoundErrorhandling, summarizing duplicate lines) is clear and consistent with other signatures.Two small nits you might consider (non‑blocking):
- If
logs_archive.get()can returnbytesin some contexts, you may want to ensurelogsisstr(e.g., decode or cast) before running the regex, to avoid accidental type issues.- If this log can be very long, you could mirror the
ApiInvalidCertificateSignaturebehavior and cap the number of lines shown (you already summarize duplicates, so this is mostly about being explicit).assisted_service_mcp/src/utils/log_analyzer/signatures/controller_failed_to_start.py (1)
21-65: Clarify when this signature should fire and tighten a couple of lookupsThe structure is good (bootstrap gate, pods.json fetch, ready/conditions tables), but a few details are worth revisiting:
- The title is “Assisted Installer Controller failed to start”, yet the signature is returned even when
readyisTrue(Line 57). That can be confusing: the content will say “The controller pod is ready” while the title claims a failure. Consider returning a result only whenreadyisFalse, or at least adjusting the title based onready.bootstrap[0]["progress"]["current_stage"](Line 26) and thecontroller_podselection (Lines 35‑39) assume specific keys and at least one matching pod. You’re catching any exception and returningNone, which avoids crashes but also hides whether the metadata/JSON shape changed vs. “no controller”. If you want clearer behavior, you could:
- Use
.get("progress", {}).get("current_stage")for a softer check, and- Explicitly handle the “no assisted-installer pod found” case instead of relying on
[0]raising.None of these are blockers, but they’d make the signature behavior easier to reason about and less surprising for consumers.
assisted_service_mcp/src/utils/log_analyzer/signatures/sno_machine_cidr_signature.py (1)
18-57: Narrow the exception scope and consider using ErrorSignatureThe core logic (SNO gate, first host inventory, default route vs machine CIDR) is sound, but the broad
except Exceptionaround the entire method makes it hard to distinguish between “input is slightly malformed” and “we have a real bug in the signature”:
- A malformed route (e.g., bad IP in
route["gateway"]) will cause the whole signature to silently returnNoneinstead of just skipping that route.- Issues like
KeyErroron"machine_networks"or"inventory"will also be swallowed, which can mask regressions in metadata shape.Consider scoping the
try/exceptmore tightly (e.g., around the JSON load and the ipaddress conversions) and leaving truly unexpected exceptions to surface.Also, since this represents an error condition and you manually construct
SignatureResultwithseverity="error", it might be more consistent with the rest of the package to inherit fromErrorSignatureand useself.create_result(...)instead of instantiatingSignatureResultdirectly.assisted_service_mcp/src/utils/log_analyzer/signatures/failed_request_triggers_host_timeout.py (1)
24-59: Logic is correct; a couple of small cleanups could improve readabilityThe matching between hosts with failed requests and timeout status is implemented cleanly. A few minor polish points you might consider:
cluster = log_analyzer.metadata(Line 25) assumes metadata is always a dict; ifmetadatacan ever beNone, a quick guard (if not cluster: return None) before.get("hosts", [])would avoid surprises.- For the log scan,
if self.LOG_PATTERN.search(agent_logs):is slightly simpler and avoids building an intermediate list compared tolen(self.LOG_PATTERN.findall(agent_logs)) > 0(Lines 37‑38).- If host entries in metadata might ever lack
"id", accessingh["id"](Lines 28, 34) will raise; if that’s not guaranteed by the data contract, switching toh.get("id")and skippingNoneIDs would make this more defensive.Functionally this is fine as-is; these are just small readability/robustness tweaks.
assisted_service_mcp/src/utils/log_analyzer/signatures/dual_stack_bad_route.py (1)
21-36: DualStackBadRoute implementation looks good; only tiny style nitsThe signature does what it says: single log read with
FileNotFoundErrorhandling and a straightforward regex check, returning a clearSignatureResulton match. This is consistent with the other error signatures.If you want to polish further (non‑blocking):
- You could rename
fatal_error_regextoFATAL_ERROR_REGEXto emphasize it’s a class-level constant.- As with other signatures, ensuring
ovnkube_logsis alwaysstr(iflogs_archive.getcan yield bytes for some archives) would make type behavior more explicit.Otherwise this looks ready.
assisted_service_mcp/src/utils/log_analyzer/signatures/dualstackr_dns_bug.py (1)
20-33: Straightforward detection; consider extracting the magic stringImplementation is clear and consistent with other signatures: one log fetch,
FileNotFoundErrorhandling, and a simple substring check to decide whether to emit a warning-level result.The only minor nit is the hard-coded
"must match public address family"string used both in code and content (Lines 26, 30). If this message ever changes upstream, having it as a single class-level constant (e.g.,MATCH_TEXT = "must match public address family") would make maintenance a bit easier.Otherwise, looks good.
assisted_service_mcp/src/utils/log_analyzer/signatures/user_managed_networking_load_balancer.py (1)
23-54: UMN load-balancer detection looks coherent; only minor defensive tweaksThis signature is nicely structured: metadata gating, controller log parsing via helpers, and a clear condition (
unhealthy_keyssubset oflb_operators) before emitting a warning-level result.A couple of small, optional tweaks you might consider:
- If
log_analyzer.metadatacan ever beNone, a quickif not metadata: return Nonebefore.get(...)calls (Lines 26, 29) would make the function more defensive.- The message for missing operators (Lines 48‑49) is helpful; if you expect this to be used in automated triage, you might also consider including the actual
unhealthy_keysset in the content for completeness (e.g., “Unhealthy operators: …”).Functionally this looks good as-is.
assisted_service_mcp/src/utils/log_analyzer/signatures/nameserver_in_cluster_network.py (1)
23-83: Detection logic is good; tighten nameserver parsing and reuse parsed IPsThe high-level flow (collect nameservers, compare against cluster CIDRs, emit one consolidated error result) is solid. Two small improvements would make it more robust:
_get_nameserversusesNAMESERVER_PATTERN = re.compile(r"^nameserver (.*)$")andsearch(Lines 23, 41, 52). This will capture everything afternameserver, including inline comments or extra whitespace (e.g.,nameserver 10.0.0.1 # comment). That causesipaddress.ip_address(ns)(Line 70) to raiseValueErrorand skip the entry. Using a stricter pattern such asr"^nameserver\s+(\S+)"andmatch()would avoid pulling in comments and trailing spaces.- In the overlap loop (Lines 67‑76), you call
ipaddress.ip_address(ns)for every(ns, cidr)pair. You could parse each nameserver once before the inner loop, skipping invalid entries early, and then reuse theipaddressobject across CIDRs. It’s minor, but simplifies the exception handling and avoids repeated parsing.These are refinements; the current implementation will still work in the common case where resolv.conf lines are simple
nameserver <ip>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py(4 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/__init__.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/components_version_signature.py(2 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/container_crash_analysis.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/controller_failed_to_start.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/controller_warnings.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/dual_stack_bad_route.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/dualstackr_dns_bug.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/duplicate_vip.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/error_creating_read_write_layer.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py(0 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/error_on_cleanup_install_device.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/events_installation_attempts.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/failed_request_triggers_host_timeout.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/ip_changed_after_reboot.py(2 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/libvirt_reboot_flag_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/machine_config_daemon_error_extracting.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/missing_mc.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/nameserver_in_cluster_network.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py(0 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/networks_mtu_mismatch.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/notes(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/release_pull_error_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/slow_image_download_signature.py(2 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/sno_hostname_has_etcd.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/sno_machine_cidr_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/user_has_logged_into_cluster.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/user_managed_networking_load_balancer.py(1 hunks)tests/utils/log_analyzer/signatures/test_container_crash_analysis.py(2 hunks)tests/utils/log_analyzer/signatures/test_slow_image_download_signature.py(1 hunks)tests/utils/log_analyzer/test_log_analyzer.py(6 hunks)tests/utils/test_auth.py(4 hunks)
💤 Files with no reviewable changes (2)
- assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py
- assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py
🧰 Additional context used
🧬 Code graph analysis (25)
assisted_service_mcp/src/utils/log_analyzer/signatures/user_has_logged_into_cluster.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
all_host_journal_logs(195-218)
assisted_service_mcp/src/utils/log_analyzer/signatures/ip_changed_after_reboot.py (1)
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/sno_machine_cidr_signature.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
cluster_is_sno(180-193)metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/dualstackr_dns_bug.py (1)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)
assisted_service_mcp/src/utils/log_analyzer/signatures/error_on_cleanup_install_device.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)generate_table(70-74)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
metadata(35-47)get_host_log_file(118-149)
assisted_service_mcp/src/utils/log_analyzer/signatures/machine_config_daemon_error_extracting.py (1)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/libvirt_reboot_flag_signature.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)generate_table(70-74)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py (1)
analyze(21-39)
assisted_service_mcp/src/utils/log_analyzer/signatures/components_version_signature.py (1)
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
tests/utils/test_auth.py (1)
assisted_service_mcp/utils/auth.py (2)
get_offline_token(10-45)get_access_token(48-117)
assisted_service_mcp/src/utils/log_analyzer/signatures/duplicate_vip.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)archive_dir_contents(77-89)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/dual_stack_bad_route.py (1)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)
assisted_service_mcp/src/utils/log_analyzer/signatures/controller_warnings.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
get_controller_logs(171-178)
assisted_service_mcp/src/utils/log_analyzer/signatures/release_pull_error_signature.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
all_host_journal_logs(195-218)
tests/utils/log_analyzer/signatures/test_slow_image_download_signature.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/slow_image_download_signature.py (2)
SlowImageDownloadSignature(14-54)analyze(22-44)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
get_last_install_cluster_events(69-80)
assisted_service_mcp/src/utils/log_analyzer/signatures/nameserver_in_cluster_network.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)archive_dir_contents(77-89)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/sno_hostname_has_etcd.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
cluster_is_sno(180-193)metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py (1)
analyze(23-39)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
get_controller_logs(171-178)
assisted_service_mcp/src/utils/log_analyzer/signatures/events_installation_attempts.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (3)
get_all_cluster_events(82-95)partition_cluster_events(98-116)get_last_install_cluster_events(69-80)
assisted_service_mcp/src/utils/log_analyzer/signatures/failed_request_triggers_host_timeout.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
metadata(35-47)get_host_log_file(118-149)
assisted_service_mcp/src/utils/log_analyzer/signatures/user_managed_networking_load_balancer.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (2)
operator_statuses_from_controller_logs(10-37)filter_operators(50-64)assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
metadata(35-47)get_controller_logs(171-178)
assisted_service_mcp/src/utils/log_analyzer/signatures/error_creating_read_write_layer.py (1)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)archive_dir_contents(77-89)create_result(110-126)
assisted_service_mcp/src/utils/log_analyzer/signatures/controller_failed_to_start.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
Signature(50-89)SignatureResult(14-47)generate_table(70-74)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/missing_mc.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
cluster_is_sno(180-193)
assisted_service_mcp/src/utils/log_analyzer/signatures/networks_mtu_mismatch.py (1)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)
🪛 GitHub Actions: Black
assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py
[error] 1-1: Black formatting check failed: 1 file would be reformatted.
🪛 GitHub Actions: Python linter
assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py
[error] 77-77: Pylint: Final newline missing (C0304) in file.
⏰ 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 (25)
assisted_service_mcp/src/utils/log_analyzer/signatures/components_version_signature.py (1)
16-28: Metadata now assumes top-levelrelease_tag/versionsonlyThe switch to
metadata.get("release_tag")/metadata.get("versions")at the root (no["cluster"]fallback) simplifies things and matches the newLogAnalyzer.metadatacomment, but it does change behavior for older bundles that might still nest these undercluster. If backward-compatibility with older archives matters, please confirm that_clean_metadata_jsonnormalizes those cases or consider a small fallback to the nested keys.tests/utils/test_auth.py (1)
42-132: Directly patching settings attributes improves test clarity—verifyget_settingpathSwitching the tests to patch
assisted_service_mcp.src.settings.settings.OFFLINE_TOKEN/SSO_URLinstead ofget_settingmakes the intent clearer and keeps the auth module logic unmocked. This assumesget_setting("OFFLINE_TOKEN"|"SSO_URL")ultimately reads from those attributes; ifget_settinghas other resolution or caching behavior, these tests might not fully exercise the intended code path. Worth double-checking thatget_settingusessettings.OFFLINE_TOKEN/settings.SSO_URLdirectly.assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py (1)
14-38: ApiInvalidCertificateSignature implementation looks solidThe signature cleanly:
- Guards missing controller logs with
FileNotFoundError.- Scans logs via a precompiled pattern and summarizes up to 5 lines plus a “N similar lines” suffix.
- Uses
create_resultfromErrorSignaturewith severity"error".This fits well with the existing signature conventions.
assisted_service_mcp/src/utils/log_analyzer/signatures/ip_changed_after_reboot.py (2)
1-3: LGTM! Docstring correctly reflects the file's scope.The updated module docstring now accurately describes that this file contains only the IpChangedAfterReboot signature, since LibvirtRebootFlagSignature was moved to its own module.
46-47: LGTM! Simplified metadata access.The direct assignment eliminates an unnecessary intermediate variable while maintaining the same functionality.
tests/utils/log_analyzer/signatures/test_container_crash_analysis.py (2)
1-3: LGTM! Docstring accurately reflects the test target.The updated docstring now specifically references ContainerCrashAnalysis, improving clarity.
13-15: LGTM! Explicit import improves maintainability.The migration from wildcard imports to explicit module paths improves code clarity and makes dependencies transparent.
assisted_service_mcp/src/utils/log_analyzer/signatures/events_installation_attempts.py (1)
24-39: Verify the last attempt retrieval aligns with partition logic.Ensure that
get_last_install_cluster_events()returns events from the same partition that this signature identifies as the "last attempt" after the fix above.assisted_service_mcp/src/utils/log_analyzer/signatures/machine_config_daemon_error_extracting.py (1)
1-41: LGTM! Well-structured signature for MCD extraction errors.The implementation correctly:
- Uses a glob pattern to locate logs across different hosts
- Gracefully handles missing log files
- Returns an appropriate warning-level result when the error pattern is detected
assisted_service_mcp/src/utils/log_analyzer/signatures/slow_image_download_signature.py (2)
1-3: LGTM! Docstring now specifically references the signature.The updated docstring improves clarity by explicitly naming SlowImageDownloadSignature.
46-54: LGTM! Conversion to instance method with cleaner implementation.The refactor from classmethod to instance method is appropriate, and the walrus operator makes the list comprehension more concise while maintaining readability.
assisted_service_mcp/src/utils/log_analyzer/signatures/user_has_logged_into_cluster.py (1)
1-35: LGTM! Clean implementation for detecting user logins.The signature correctly:
- Uses an appropriate regex pattern to detect PAM session openings
- Leverages the
all_host_journal_logs()iterator to process all hosts- Aggregates findings across multiple hosts
- Returns a warning-level result when evidence is found
assisted_service_mcp/src/utils/log_analyzer/signatures/error_on_cleanup_install_device.py (1)
1-48: LGTM! Well-structured signature for cleanupInstallDevice errors.The implementation correctly:
- Detects non-fatal errors with an appropriate regex pattern
- Iterates over all hosts in the cluster metadata
- Gracefully handles missing installer logs
- Uses OrderedDict to maintain consistent table column ordering
- Provides helpful context about the non-fatal nature of the error
assisted_service_mcp/src/utils/log_analyzer/signatures/networks_mtu_mismatch.py (1)
1-34: LGTM! Effective signature for detecting MTU mismatches.The implementation correctly:
- Uses a precise regex pattern to extract both MTU values
- Employs a glob pattern to locate SDN logs within the must-gather archive
- Provides a clear error message explaining the mismatch
- Returns error severity, which is appropriate for this networking configuration issue
tests/utils/log_analyzer/signatures/test_slow_image_download_signature.py (1)
13-231: Strong, thorough coverage for SlowImageDownloadSignature testsThe helper event generator matches the implementation’s regex format, and the suite exercises all key behaviors: normal vs slow downloads, threshold edge cases, absence/malformed events, exception handling, invalid download-rate formats, and multiple slow entries per host, plus simple checks on
nameandminimum_download_rate_mb. This gives very solid regression protection for the signature with no obvious gaps that need addressing now.assisted_service_mcp/src/utils/log_analyzer/signatures/controller_warnings.py (1)
14-36: ControllerWarnings implementation is clear and appropriateThe signature cleanly handles missing controller logs, extracts warning lines with a simple regex, and caps output with a “+N additional warnings” summary, which is a good balance between signal and verbosity. Returning a
SignatureResultwith warning severity only when warnings are present is consistent with the rest of the signatures.assisted_service_mcp/src/utils/log_analyzer/signatures/duplicate_vip.py (1)
41-67: Double‑check type returned fromlogs_archive.get(...ip-addr.txt)This logic assumes that
ip_addrandbootstrap_ip_addrare text strings so thatvip in ip_addrworks correctly. Iflogs_archive.get()can return bytes for these files, this will either raise aTypeErroror silently fail the membership test depending on how it’s used elsewhere.It’s worth confirming the expected type for these paths and normalizing if needed, e.g.:
ip_addr = log_analyzer.logs_archive.get(...) if isinstance(ip_addr, bytes): ip_addr = ip_addr.decode("utf-8", errors="ignore")so the substring check is always safe.
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (3)
43-43: LGTM! Metadata structure flattened to root level.The metadata handling has been successfully refactored to operate on root-level keys instead of nested cluster keys. This simplifies the data structure and aligns with the PR's cleanup objectives.
Also applies to: 52-52, 59-65
180-193: LGTM! Clear SNO detection with safe defaults.The method appropriately handles exceptions by returning False as a safe default, which is reasonable for a boolean helper used in signature detection.
195-218: LGTM! Well-structured iterator with proper error handling.The iterator cleanly handles missing journal logs and metadata errors, using early returns and continue statements appropriately.
tests/utils/log_analyzer/test_log_analyzer.py (2)
25-29: LGTM! Test updated to match new metadata structure.The test correctly validates the flattened metadata structure with root-level keys, consistent with the changes in
log_analyzer.py.Also applies to: 49-49, 61-61
112-114: LGTM! Imports updated for reorganized signature modules.The explicit import paths reflect the new module structure where each signature class has its own file, improving code organization and maintainability.
Also applies to: 130-132, 148-150
assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
10-77: Helper functions are well-designed with proper error handling.The helper functions provide clean abstractions for operator status parsing and hostname extraction, with appropriate fallbacks for missing data.
assisted_service_mcp/src/utils/log_analyzer/signatures/container_crash_analysis.py (1)
1-11: LGTM! Module successfully refactored for single responsibility.The file now focuses solely on ContainerCrashAnalysis, with helper functions moved to
helpers.pyand other signatures split into their own modules. This improves code organization and maintainability.assisted_service_mcp/src/utils/log_analyzer/signatures/__init__.py (1)
10-46: LGTM! Excellent refactor from wildcard to explicit imports.Replacing wildcard imports with explicit imports improves code maintainability, IDE support, and makes dependencies clear. The
# noqa: F401comments are appropriate since these classes are consumed dynamically viainspect.getmembers().
| def analyze(self, log_analyzer) -> Optional[SignatureResult]: | ||
| """Analyze multiple installation attempts.""" | ||
| try: | ||
| # Get all cluster events and partition them by reset events | ||
| all_events = log_analyzer.get_all_cluster_events() | ||
| partitions = log_analyzer.partition_cluster_events(all_events) | ||
| installation_attempts = len(partitions) | ||
|
|
There was a problem hiding this comment.
Critical logic bug: get_all_cluster_events() returns only the last partition.
The code calls get_all_cluster_events() expecting to receive all events, but according to the relevant code snippet from log_analyzer.py (lines 81-94), that method returns self._cluster_events = self.partition_cluster_events(all_events)[-1] — only the last partition. Calling partition_cluster_events() on that result will always return a single partition, so installation_attempts will always be 1, and this signature will never detect multiple installation attempts.
Apply this diff to load all events directly:
def analyze(self, log_analyzer) -> Optional[SignatureResult]:
"""Analyze multiple installation attempts."""
try:
- # Get all cluster events and partition them by reset events
- all_events = log_analyzer.get_all_cluster_events()
- partitions = log_analyzer.partition_cluster_events(all_events)
+ # Load all events directly from the archive (not just last partition)
+ import json
+ from typing import cast
+ events_content = log_analyzer.logs_archive.get("cluster_events.json")
+ all_events = json.loads(cast(str | bytes, events_content))
+
+ # Partition events by reset events to count installation attempts
+ partitions = log_analyzer.partition_cluster_events(all_events)
installation_attempts = len(partitions)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def analyze(self, log_analyzer) -> Optional[SignatureResult]: | |
| """Analyze multiple installation attempts.""" | |
| try: | |
| # Get all cluster events and partition them by reset events | |
| all_events = log_analyzer.get_all_cluster_events() | |
| partitions = log_analyzer.partition_cluster_events(all_events) | |
| installation_attempts = len(partitions) | |
| def analyze(self, log_analyzer) -> Optional[SignatureResult]: | |
| """Analyze multiple installation attempts.""" | |
| try: | |
| # Load all events directly from the archive (not just last partition) | |
| import json | |
| from typing import cast | |
| events_content = log_analyzer.logs_archive.get("cluster_events.json") | |
| all_events = json.loads(cast(str | bytes, events_content)) | |
| # Partition events by reset events to count installation attempts | |
| partitions = log_analyzer.partition_cluster_events(all_events) | |
| installation_attempts = len(partitions) |
🤖 Prompt for AI Agents
In
assisted_service_mcp/src/utils/log_analyzer/signatures/events_installation_attempts.py
around lines 16 to 23, replace the call to log_analyzer.get_all_cluster_events()
(which returns only the last partition) with a call that loads the full event
list (e.g., log_analyzer.load_all_cluster_events() or the equivalent method that
returns all events), then pass that full event list into
log_analyzer.partition_cluster_events(...) so installation_attempts reflects the
true number of partitions; ensure the variable names remain consistent and
handle None/empty results as before.
| """ | ||
| Helper functions for signature analysis. | ||
| """ | ||
|
|
||
| import json | ||
| import re | ||
| from typing import Any, Generator, Callable, Dict | ||
|
|
||
|
|
||
| def operator_statuses_from_controller_logs( | ||
| controller_log: str, include_empty: bool = False | ||
| ): | ||
| operator_regex = re.compile(r"Operator ([a-z\-]+), statuses: \[(.*)\].*") | ||
| conditions_regex = re.compile(r"\{(.+?)\}") | ||
| condition_regex = re.compile( | ||
| r"([A-Za-z]+) (False|True) ([0-9a-zA-Z\-]+ [0-9a-zA-Z\:]+ [0-9a-zA-Z\-\+]+ [A-Z]+) (.*)" | ||
| ) | ||
| operator_statuses = {} | ||
|
|
||
| for operator_name, operator_status in operator_regex.findall(controller_log): | ||
| if include_empty: | ||
| operator_statuses[operator_name] = {} | ||
| operator_conditions = operator_statuses.setdefault(operator_name, {}) | ||
| for operator_conditions_raw in conditions_regex.findall(operator_status): | ||
| for ( | ||
| condition_name, | ||
| condition_result, | ||
| condition_timestamp, | ||
| condition_reason, | ||
| ) in condition_regex.findall(operator_conditions_raw): | ||
| operator_conditions[condition_name] = { | ||
| "result": condition_result == "True", | ||
| "timestamp": condition_timestamp, | ||
| "reason": condition_reason, | ||
| } | ||
|
|
||
| return operator_statuses | ||
|
|
||
|
|
||
| def condition_has_result( | ||
| operator_conditions, expected_condition_name: str, expected_condition_result: bool | ||
| ) -> bool: | ||
| return any( | ||
| condition_values["result"] == expected_condition_result | ||
| for condition_name, condition_values in operator_conditions.items() | ||
| if condition_name == expected_condition_name | ||
| ) | ||
|
|
||
|
|
||
| def filter_operators( | ||
| operator_statuses, | ||
| required_conditions, | ||
| aggregation_function: Callable[[Generator[Any, None, None]], bool], | ||
| ): | ||
| return { | ||
| operator_name: operator_conditions | ||
| for operator_name, operator_conditions in operator_statuses.items() | ||
| if aggregation_function( | ||
| condition_has_result( | ||
| operator_conditions, required_condition_name, expected_condition_result | ||
| ) | ||
| for required_condition_name, expected_condition_result in required_conditions | ||
| ) | ||
| } | ||
|
|
||
|
|
||
| def get_hostname(host: Dict[str, Any]) -> str: | ||
| """Extract hostname from host metadata.""" | ||
| hostname = host.get("requested_hostname") | ||
| if hostname: | ||
| return hostname | ||
|
|
||
| try: | ||
| inventory = json.loads(host["inventory"]) | ||
| return inventory["hostname"] | ||
| except (KeyError, json.JSONDecodeError): | ||
| return host.get("id", "unknown") No newline at end of file |
There was a problem hiding this comment.
Fix formatting and add final newline.
The pipeline is failing due to Black formatting issues and a missing final newline. These must be resolved before merging.
Run Black to reformat the file and ensure it ends with a newline:
#!/bin/bash
# Reformat the file with Black
black assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py🧰 Tools
🪛 GitHub Actions: Black
[error] 1-1: Black formatting check failed: 1 file would be reformatted.
🪛 GitHub Actions: Python linter
[error] 77-77: Pylint: Final newline missing (C0304) in file.
🤖 Prompt for AI Agents
In assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py around
lines 1 to 77, the file fails CI due to Black formatting and a missing final
newline; run Black (e.g., black
assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py) to reformat
the file and then ensure the file ends with a single trailing newline (save
after formatting) before committing.
There was a problem hiding this comment.
Is this file meant to be added to this PR?
There was a problem hiding this comment.
lol nope. Maybe I should split this into smaller chunks 😅
This will make it easier to add new signatures and will make it more clear where to find helper functions if/when those are needed.
This was added as an easy way to maintain compatibility with older signatures, but it just adds complexity and can be removed
Adds helpers for checking if a cluster is SNO and for iterating through all the hosts journal logs. These resolve some pylint duplicate code errors.
Since it doesn't use and isn't used by the analyzer class it's a better match for the utils file
Match the source directory structure in tests This makes things like test_helpers.py clear when we have multiple helpers.py files in the project
After the move a change in import order caused this test to start failing. This is because the settings are a global concept and end up shared across tests. This should be fixed more generally, but for now just isolate the import of auth in the auth tests to address the test-specific problem.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/events_installation_attempts.py (1)
16-25: Logic bug: usingget_all_cluster_events()means installation_attempts is always 1
log_analyzer.get_all_cluster_events()currently returns only the last partition of events, not the full event list. Callingpartition_cluster_events(all_events)on that result will always give a single partition, soinstallation_attemptswill always be 1 and this signature will never flag multiple attempts. This is the same issue noted in earlier review feedback.To base the count on the real number of attempts, load the full event list from
cluster_events.jsonand partition that, then derive the last attempt from those partitions instead of callingget_all_cluster_events():- def analyze(self, log_analyzer) -> Optional[SignatureResult]: - """Analyze multiple installation attempts.""" - try: - # Get all cluster events and partition them by reset events - all_events = log_analyzer.get_all_cluster_events() - partitions = log_analyzer.partition_cluster_events(all_events) - installation_attempts = len(partitions) + def analyze(self, log_analyzer) -> Optional[SignatureResult]: + """Analyze multiple installation attempts.""" + try: + # Load all cluster events and partition them by reset events + import json + + events_content = log_analyzer.logs_archive.get("cluster_events.json") + all_events = json.loads(events_content) + partitions = log_analyzer.partition_cluster_events(all_events) + installation_attempts = len(partitions) @@ - if installation_attempts != 1: - current_events = log_analyzer.get_last_install_cluster_events() + if installation_attempts != 1: + current_events = partitions[-1] if partitions else []assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
1-77: Address the formatting issues flagged in previous review.The Black formatting and missing final newline issues identified in the previous review must be resolved before merging.
Based on past review comments.
🧹 Nitpick comments (10)
assisted_service_mcp/src/utils/log_analyzer/signatures/nameserver_in_cluster_network.py (1)
11-13: Use relative import for consistency.The absolute import path
from assisted_service_mcp.src.utils.log_analyzer.log_analyzer importshould be a relative import for consistency with other signature files in this module.Apply this diff:
-from assisted_service_mcp.src.utils.log_analyzer.log_analyzer import ( - LOG_BUNDLE_PATH, -) +from ...log_analyzer import LOG_BUNDLE_PATHOr simply:
-from assisted_service_mcp.src.utils.log_analyzer.log_analyzer import ( - LOG_BUNDLE_PATH, -) +from ..log_analyzer import LOG_BUNDLE_PATHassisted_service_mcp/src/utils/log_analyzer/signatures/dualstackr_dns_bug.py (1)
8-10: Use relative import for consistency.The absolute import path should be a relative import to match the pattern used in other signature files.
Apply this diff:
-from assisted_service_mcp.src.utils.log_analyzer.log_analyzer import ( - LOG_BUNDLE_PATH, -) +from ...log_analyzer import LOG_BUNDLE_PATHassisted_service_mcp/src/utils/log_analyzer/signatures/error_on_cleanup_install_device.py (1)
21-27: Consider defensively handling missing metadata before iterating hosts
cluster = log_analyzer.metadatais assumed to be a mapping, andcluster.get("hosts", [])will fail if metadata is everNoneor otherwise not a dict (e.g., if_clean_metadata_jsonreturnsNone). You could early‑return when metadata is falsy to avoid noisy exceptions from this signature.def analyze(self, log_analyzer) -> Optional[SignatureResult]: - cluster = log_analyzer.metadata + cluster = log_analyzer.metadata + if not cluster: + return None hosts = [] for host in cluster.get("hosts", []):assisted_service_mcp/src/utils/log_analyzer/signatures/sno_hostname_has_etcd.py (1)
17-33: Title wording and host access could be clarifiedThis fires when
"etcd"is present in the SNO hostname, but the title reads"No etcd in SNO hostname", which can be confusing when rendered in reports. Also,cluster["hosts"]will raise if metadata is unexpectedlyNoneor missinghosts, even though you already treat such cases as non-fatal.You could consider:
- cluster = log_analyzer.metadata - if len(cluster["hosts"]) != 1: + cluster = log_analyzer.metadata + if not cluster or len(cluster.get("hosts", [])) != 1: return None @@ - return self.create_result( - title="No etcd in SNO hostname", content=content, severity="error" - ) + return self.create_result( + title="Etcd detected in SNO hostname", + content=content, + severity="error", + )assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py (1)
18-38: LGTM: consistent with other certificate signatures and handles missing logs safelyThe path, regex, and “first line plus count of additional similar lines” behavior mirror
ApiInvalidCertificateSignatureand correctly guard on missingkube-apiserver.log. If you ever findlogs_archive.getreturning bytes here, a smalldecode()/str()normalization step beforefindall()would make the function more defensive, but it’s not strictly required given current usage.assisted_service_mcp/src/utils/log_analyzer/signatures/missing_mc.py (1)
17-38: Decode error handling could be more informative and narrowly scopedThe SNO guard, path, and regex are fine, but the broad
except Exception:aroundraw.decode("utf-8")silently drops the signature on any decoding issue (including programmer errors likerawnot being bytes). That can make troubleshooting harder if the bundle format changes.You could narrow and log the failure:
- try: - text = raw.decode("utf-8") - except Exception: - return None + try: + text = raw.decode("utf-8") + except (AttributeError, UnicodeDecodeError) as e: + logger.warning("Unable to decode MachineConfigPool yaml as UTF-8: %s", e) + return Noneassisted_service_mcp/src/utils/log_analyzer/signatures/duplicate_vip.py (1)
22-87: Duplicate VIP detection logic looks correct; message text could be slightly clearerThe flow (skip SNO/UMN, gather VIPs from
api_vips, scan control-plane and bootstrapip-addr.txt, then aggregate by VIP) is sound and handles missing directories/files gracefully viaFileNotFoundErrorandarchive_dir_contents. Using a set per VIP also avoids duplicate node entries.One small nit: when bootstrap participates in a collision, the message
in control-plane hosts ...can be slightly misleading since"bootstrap"is included in the host list. If you care about wording, you could relax that phrasing to “in nodes …” or similar, but the behavior is otherwise solid.assisted_service_mcp/src/utils/log_analyzer/signatures/error_creating_read_write_layer.py (1)
51-52: Overly broad exception handling masks potential issues.The bare
except Exceptionsilently continues on any parsing or processing error. While resilience is valuable, this hides bugs and makes debugging difficult. Consider logging the exception before continuing, or narrowing to specific expected exceptions (e.g.,yaml.YAMLError).Apply this diff to log exceptions before continuing:
- except Exception: - continue + except Exception as e: + logger.debug("Failed to parse pods.yaml in namespace %s: %s", namespace_dir, e) + continueassisted_service_mcp/src/utils/log_analyzer/signatures/failed_request_triggers_host_timeout.py (1)
37-38: Simplify pattern matching check.Using
len(self.LOG_PATTERN.findall(agent_logs)) > 0is less efficient and less idiomatic thanself.LOG_PATTERN.search(agent_logs). Thesearchmethod stops at the first match, whilefindallscans the entire log.Apply this diff:
- if len(self.LOG_PATTERN.findall(agent_logs)) > 0: + if self.LOG_PATTERN.search(agent_logs): failed_requests_hosts.add(host["id"])assisted_service_mcp/src/utils/log_analyzer/signatures/user_managed_networking_load_balancer.py (1)
8-13: Use consistent import style.Lines 8-11 use absolute imports while line 13 uses relative imports. For consistency and maintainability, use relative imports throughout the module.
Apply this diff:
-from assisted_service_mcp.src.utils.log_analyzer.signatures.helpers import ( - operator_statuses_from_controller_logs, - filter_operators, -) - from .base import ErrorSignature, SignatureResult +from .helpers import operator_statuses_from_controller_logs, filter_operators
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py(4 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/__init__.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/components_version_signature.py(2 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/container_crash_analysis.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/controller_failed_to_start.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/controller_warnings.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/dual_stack_bad_route.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/dualstackr_dns_bug.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/duplicate_vip.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/error_creating_read_write_layer.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py(0 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/error_on_cleanup_install_device.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/events_installation_attempts.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/failed_request_triggers_host_timeout.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/ip_changed_after_reboot.py(2 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/libvirt_reboot_flag_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/machine_config_daemon_error_extracting.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/missing_mc.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/nameserver_in_cluster_network.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py(0 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/networks_mtu_mismatch.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/release_pull_error_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/slow_image_download_signature.py(2 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/sno_hostname_has_etcd.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/sno_machine_cidr_signature.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/user_has_logged_into_cluster.py(1 hunks)assisted_service_mcp/src/utils/log_analyzer/signatures/user_managed_networking_load_balancer.py(1 hunks)tests/utils/log_analyzer/signatures/test_container_crash_analysis.py(2 hunks)tests/utils/log_analyzer/signatures/test_slow_image_download_signature.py(1 hunks)tests/utils/log_analyzer/test_log_analyzer.py(6 hunks)tests/utils/test_auth.py(4 hunks)
💤 Files with no reviewable changes (2)
- assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py
- assisted_service_mcp/src/utils/log_analyzer/signatures/networking.py
🚧 Files skipped from review as they are similar to previous changes (8)
- assisted_service_mcp/src/utils/log_analyzer/signatures/machine_config_daemon_error_extracting.py
- tests/utils/log_analyzer/signatures/test_slow_image_download_signature.py
- assisted_service_mcp/src/utils/log_analyzer/signatures/controller_failed_to_start.py
- assisted_service_mcp/src/utils/log_analyzer/signatures/dual_stack_bad_route.py
- assisted_service_mcp/src/utils/log_analyzer/signatures/networks_mtu_mismatch.py
- assisted_service_mcp/src/utils/log_analyzer/signatures/sno_machine_cidr_signature.py
- assisted_service_mcp/src/utils/log_analyzer/signatures/controller_warnings.py
- tests/utils/test_auth.py
🧰 Additional context used
🧬 Code graph analysis (17)
assisted_service_mcp/src/utils/log_analyzer/signatures/failed_request_triggers_host_timeout.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
metadata(35-47)get_host_log_file(118-149)assisted_service_mcp/src/utils/log_analyzer/signatures/advanced_analysis.py (1)
FailedRequestTriggersHostTimeout(171-217)
assisted_service_mcp/src/utils/log_analyzer/signatures/error_on_cleanup_install_device.py (4)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)generate_table(70-74)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
metadata(35-47)get_host_log_file(118-149)assisted_service_mcp/src/utils/log_analyzer/signatures/error_detection.py (1)
ErrorOnCleanupInstallDevice(137-169)
assisted_service_mcp/src/utils/log_analyzer/signatures/libvirt_reboot_flag_signature.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)generate_table(70-74)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/missing_mc.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
cluster_is_sno(180-193)
assisted_service_mcp/src/utils/log_analyzer/signatures/ip_changed_after_reboot.py (1)
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/release_pull_error_signature.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
all_host_journal_logs(195-218)
assisted_service_mcp/src/utils/log_analyzer/signatures/dualstackr_dns_bug.py (1)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)
assisted_service_mcp/src/utils/log_analyzer/signatures/duplicate_vip.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)archive_dir_contents(77-89)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/error_creating_read_write_layer.py (1)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)archive_dir_contents(77-89)create_result(110-126)
assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py (1)
analyze(23-39)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
get_controller_logs(171-178)
assisted_service_mcp/src/utils/log_analyzer/signatures/components_version_signature.py (1)
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py (1)
analyze(21-39)
assisted_service_mcp/src/utils/log_analyzer/signatures/user_managed_networking_load_balancer.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (2)
operator_statuses_from_controller_logs(10-37)filter_operators(50-64)assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
metadata(35-47)get_controller_logs(171-178)
assisted_service_mcp/src/utils/log_analyzer/signatures/sno_hostname_has_etcd.py (3)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (3)
ErrorSignature(92-126)SignatureResult(14-47)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/signatures/helpers.py (1)
get_hostname(67-77)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (2)
cluster_is_sno(180-193)metadata(35-47)
assisted_service_mcp/src/utils/log_analyzer/signatures/events_installation_attempts.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (3)
get_all_cluster_events(82-95)partition_cluster_events(98-116)get_last_install_cluster_events(69-80)
assisted_service_mcp/src/utils/log_analyzer/signatures/user_has_logged_into_cluster.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (2)
Signature(50-89)SignatureResult(14-47)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
all_host_journal_logs(195-218)
assisted_service_mcp/src/utils/log_analyzer/signatures/nameserver_in_cluster_network.py (2)
assisted_service_mcp/src/utils/log_analyzer/signatures/base.py (4)
ErrorSignature(92-126)SignatureResult(14-47)archive_dir_contents(77-89)create_result(110-126)assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
metadata(35-47)
⏰ 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)
assisted_service_mcp/src/utils/log_analyzer/signatures/ip_changed_after_reboot.py (1)
47-47: LGTM! Metadata access simplified.The direct access to
log_analyzer.metadataaligns with the refactored metadata structure that now exposes cluster information at the root level.assisted_service_mcp/src/utils/log_analyzer/signatures/slow_image_download_signature.py (1)
46-54: LGTM! Clean refactor to instance method.The conversion from classmethod to instance method and the use of the walrus operator in the list comprehension makes the code more concise while maintaining readability.
assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py (1)
27-38: LGTM! Good error handling and output limiting.The implementation correctly handles missing files and limits output to the first 5 matching lines to prevent overwhelming results, consistent with similar signatures in the codebase.
assisted_service_mcp/src/utils/log_analyzer/signatures/libvirt_reboot_flag_signature.py (1)
19-52: LGTM! Logic is sound for detecting the libvirt reboot flag issue.The signature correctly:
- Skips SNO clusters (not relevant)
- Filters hosts with specific conditions (1 disk, KVM, Rebooting stage, error status)
- Excludes bootstrap nodes
- Returns a warning only when all-but-one host is affected
assisted_service_mcp/src/utils/log_analyzer/signatures/nameserver_in_cluster_network.py (2)
25-57: LGTM! Robust error handling for missing resolv.conf files.The method correctly handles missing files at both control-plane and bootstrap levels, accumulating nameservers across all nodes.
59-84: LGTM! Correct use of strict=False for CIDR validation.The
strict=Falseparameter inip_network()is appropriate here since we're checking if an IP belongs to a network, not validating CIDR notation strictness.assisted_service_mcp/src/utils/log_analyzer/signatures/dualstackr_dns_bug.py (1)
20-34: LGTM! Simple and effective detection.The substring search for "must match public address family" is straightforward and appropriate for detecting this specific issue.
assisted_service_mcp/src/utils/log_analyzer/signatures/components_version_signature.py (1)
23-27: LGTM! Correctly updated for root-level metadata access.The changes align with the metadata refactor where cluster information is now at the root level rather than nested under a "cluster" key.
tests/utils/log_analyzer/signatures/test_container_crash_analysis.py (2)
2-2: LGTM! Docstring updated for clarity.The docstring now specifically references the
ContainerCrashAnalysissignature rather than the generic "advanced analysis" context.
13-15: LGTM! Import path correctly updated.The import path reflects the reorganization where
ContainerCrashAnalysisis now in its own dedicated module rather than the broaderadvanced_analysismodule.assisted_service_mcp/src/utils/log_analyzer/signatures/user_has_logged_into_cluster.py (1)
14-34: LGTM: signature cleanly reusesall_host_journal_logsand emits concise warningsThe logic is straightforward, resilient to missing logs via
all_host_journal_logs(), and the per‑host messaging is clear. No changes needed.tests/utils/log_analyzer/test_log_analyzer.py (1)
25-29: Tests correctly reflect new metadata layout and signature locationsThe updated
mdfixture,deleted_hostsassertion, and the imports forComponentsVersionSignature,SNOHostnameHasEtcd, andSNOMachineCidrSignatureall match the new root-level metadata schema and per-module signature structure. The tests exercise the updated paths without over-constraining behavior.Also applies to: 49-49, 61-62, 112-115, 130-143, 148-160
assisted_service_mcp/src/utils/log_analyzer/signatures/release_pull_error_signature.py (1)
15-34: LGTM!The signature correctly identifies release image pull failures by scanning journal logs and produces clear error messages with host identification.
assisted_service_mcp/src/utils/log_analyzer/log_analyzer.py (1)
180-218: LGTM!The new helper methods are well-designed:
cluster_is_snohas appropriate exception handlingall_host_journal_logselegantly handles missing log files with the iterator patternThe metadata refactoring to root-level fields simplifies access patterns.
assisted_service_mcp/src/utils/log_analyzer/signatures/container_crash_analysis.py (1)
2-2: LGTM!The updated module docstring and import adjustments improve clarity without changing functionality.
Also applies to: 11-11
assisted_service_mcp/src/utils/log_analyzer/signatures/__init__.py (1)
10-46: LGTM!Replacing wildcard imports with explicit imports improves code clarity and enables better static analysis while maintaining the dynamic signature discovery mechanism.
| LOG_PATTERN = re.compile( | ||
| 'time=".*" level=error msg=".*x509: certificate is valid.* not .*' | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the regex pattern is complete.
The LOG_PATTERN regex appears to be missing a closing quote or pattern marker. The pattern ends with 'time=".*" level=error msg=".*x509: certificate is valid.* not .*' but typically x509 certificate error messages have more structure.
Please verify this pattern matches the actual log format. Compare with similar patterns in the codebase (e.g., ApiExpiredCertificateSignature) to ensure consistency.
🏁 Script executed:
#!/bin/bash
# Search for similar x509 certificate error patterns in signature files
rg -n -A2 -B2 'x509.*certificate' --type=py -g 'signatures/*.py'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Find the target file and similar signature files
fd 'signature' -e py -p 'log_analyzer' 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Also search more broadly for files containing 'x509' in the codebase
rg -l 'x509' --type=pyLength of output: 183
🏁 Script executed:
#!/bin/bash
# Read the target file
cat assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.pyLength of output: 1243
🏁 Script executed:
#!/bin/bash
# Read the comparison file for similar pattern
cat assisted_service_mcp/src/utils/log_analyzer/signatures/api_expired_certificate_signature.pyLength of output: 1307
The regex pattern is missing a closing quote for the message field.
The pattern 'time=".*" level=error msg=".*x509: certificate is valid.* not .*' opens the message field with msg=" but never closes it. While the Python string syntax is valid, the regex pattern itself is structurally incomplete. It should include a closing quote to properly match the log structure:
LOG_PATTERN = re.compile(
'time=".*" level=error msg=".*x509: certificate is valid.* not .*"'
)Compare this to api_expired_certificate_signature.py which uses a simpler pattern; the invalid certificate pattern requires closing the message field quote to properly validate against actual log output.
🤖 Prompt for AI Agents
In
assisted_service_mcp/src/utils/log_analyzer/signatures/api_invalid_certificate_signature.py
around lines 17 to 19, the regex pattern opens the msg=" field but never closes
it; update the LOG_PATTERN string to include the closing double-quote after the
final .* (i.e. end the regex with ... not .*") so the pattern matches the full
msg="..."; ensure the string quoting in Python remains correct (escape if
needed) and run tests to verify it matches the intended log lines.
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit e6e0924
into
openshift-assisted:master
2 participants
Each commit is pretty self-contained and since this involves some moving stuff around I'd suggest looking at them one at a time.
The most significant changes were splitting the signatures into their own files, and changing the test files to match the source structure.
If this is too much though I can split this into separate PRs.
Summary by CodeRabbit
Release Notes