Ocm support#175
Conversation
Signed-off-by: r2dedios <alex.ansi.c@gmail.com>
Signed-off-by: r2dedios <alex.ansi.c@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: r2dedios The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @r2dedios! It looks like this is your first PR to openshift-assisted/assisted-service-mcp 🎉 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @r2dedios. Thanks for your PR. I'm waiting for a openshift-assisted member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
📝 WalkthroughWalkthroughThe pull request adds support for "stdio" transport mode alongside existing HTTP-based transports (SSE, streamable-http). The InventoryClient now dynamically routes between OCM and Assisted Installer APIs based on the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
openshift-ci
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
assisted_service_mcp/src/service_client/assisted_service_api.py (2)
158-161: URL substring detection for API routing is fragile.Using
"clusters_mgmt" in self.inventory_urlfor API detection could match unintended URLs (e.g., a path segment, query param, or debug endpoint containing this string). Consider a more explicit configuration flag or stricter URL path matching.♻️ Consider using a dedicated configuration flag or stricter check
+ def _is_ocm_api(self) -> bool: + """Check if the inventory URL points to OCM API.""" + parsed = urlparse(self.inventory_url) + return "/clusters_mgmt/" in parsed.path + `@sanitize_exceptions` async def get_cluster( self, cluster_id: str, get_unregistered_clusters: bool = False ) -> models.Cluster | dict[str, Any]: ... log.info("Getting cluster %s from %s", cluster_id, self.inventory_url) # Detect API type based on URL - if "clusters_mgmt" in self.inventory_url: + if self._is_ocm_api():Alternatively, add an explicit
API_TYPEsetting (e.g.,"assisted-installer"|"ocm") for deterministic routing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assisted_service_mcp/src/service_client/assisted_service_api.py` around lines 158 - 161, The substring check "clusters_mgmt" on self.inventory_url is fragile; update the routing in the method that returns clusters (the block that currently calls self.ocm_get_cluster(cluster_id)) to use a deterministic signal instead: either read a new explicit configuration flag/API_TYPE (e.g., "ocm" vs "assisted-installer") and branch on that, or perform a stricter URL match (e.g., parse self.inventory_url and compare the normalized path or hostname rather than using "in"). Ensure the change replaces the fragile if "clusters_mgmt" in self.inventory_url condition and still calls ocm_get_cluster(cluster_id) when API_TYPE indicates OCM, otherwise falls back to the existing assisted-installer flow.
617-620: OCM methods bypass_api_call, missing latency metrics.The new
ocm_list_clustersandocm_get_clustermethods callasyncio.to_thread(requests.get, ...)directly instead of using the_api_callwrapper. This means OCM API calls won't be tracked inAPI_CALL_LATENCYmetrics, creating observability inconsistency with Assisted Installer API calls.♻️ Wrap OCM HTTP calls with latency tracking
+ async def _ocm_request(self, method: str, url: str) -> requests.Response: + """Execute OCM API request with latency tracking.""" + headers = {"Authorization": f"Bearer {self.access_token}"} + with API_CALL_LATENCY.labels(api_method=f"ocm_{method}").time(): + response = await asyncio.to_thread( + requests.get, url, headers=headers, timeout=30 + ) + return response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assisted_service_mcp/src/service_client/assisted_service_api.py` around lines 617 - 620, The OCM methods ocm_list_clusters and om_get_cluster bypass the shared HTTP wrapper and thus miss API_CALL_LATENCY metrics; update both functions to call the existing _api_call wrapper instead of directly calling asyncio.to_thread(requests.get, ...). Pass the HTTP method ("GET"), target url, headers and timeout into _api_call (and any expected response handling), and adapt the call sites to use the returned response object from _api_call so error handling and latency metrics remain consistent with other Assisted Installer API calls.assisted_service_mcp/src/main.py (1)
25-28: Consider metrics accessibility in stdio mode.Metrics are initialized (
initiate_metrics) but the/metricsendpoint is not exposed in stdio mode. Internal tracking still works, but the metrics won't be externally accessible. If this is intentional (e.g., metrics are collected but not exposed to users), consider adding a log message clarifying this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assisted_service_mcp/src/main.py` around lines 25 - 28, After initiating metrics with server.list_tools_sync() and initiate_metrics(tool_names), add a check for stdio mode and emit a clear log message stating that although metrics are initialized, the /metrics endpoint is not exposed in stdio mode (or, alternatively, enable the endpoint if desired). Locate the initialization block around server.list_tools_sync() / initiate_metrics and add a conditional using whatever runtime flag or method indicates stdio mode (e.g., an is_stdio or mode/config flag) to call log.info("Metrics initialized for %d tools but /metrics endpoint is not exposed in stdio mode", len(tool_names)) so users know metrics are not externally accessible.assisted_service_mcp/src/api.py (1)
18-22:app = Nonefor stdio could cause import-time issues.Setting
app = NonewhenTRANSPORT == "stdio"may causeAttributeErrorif other modules importappand call methods on it without checking the transport mode first. Themain.pyguards this correctly, but consider adding a module-level docstring warning or using a sentinel pattern.The current approach works because
main.pycheckssettings.TRANSPORTbefore usingapp. The test suite also avoids stdio in parametrization (per context snippet). This is acceptable but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assisted_service_mcp/src/api.py` around lines 18 - 22, The module currently sets app = None when settings.TRANSPORT == "stdio", which can cause AttributeError for imports that assume app is an ASGI/HTTP app; update the module to either (a) add a clear module-level docstring noting that app may be a sentinel when TRANSPORT == "stdio" and that callers must check settings.TRANSPORT before using app, or (b) replace the bare None with a small sentinel object (e.g., _StdioSentinel or DummyApp) that exposes minimal attributes or raises a clear, descriptive error when accessed, and update any references in main.py to rely on settings.TRANSPORT instead of assuming app is a real app; ensure the chosen approach references the existing symbols app and settings.TRANSPORT so imports won't accidentally call methods on None.assisted_service_mcp/src/tools/cluster_tools.py (1)
111-111: Complex status extraction logic is hard to read.The nested ternary/fallback chain for
statusis difficult to parse at a glance:status = cluster.get("status") if isinstance(cluster.get("status"), str) else cluster.get("status", {}).get("state", cluster.get("state", "Unknown"))♻️ Extract to a helper for clarity
+def _extract_cluster_status(cluster: dict) -> str: + """Extract status from cluster dict, handling both API formats.""" + status = cluster.get("status") + if isinstance(status, str): + return status + if isinstance(status, dict): + return status.get("state", "Unknown") + return cluster.get("state", "Unknown") + `@track_tool_usage`() async def list_clusters(get_access_token_func: Callable[[], str]) -> str: ... - status = cluster.get("status") if isinstance(cluster.get("status"), str) else cluster.get("status", {}).get("state", cluster.get("state", "Unknown")) + status = _extract_cluster_status(cluster)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assisted_service_mcp/src/tools/cluster_tools.py` at line 111, The complex inline status extraction should be moved into a small helper function (e.g., get_cluster_status(cluster)) to improve readability: implement get_cluster_status to return cluster.get("status") if it's a str, otherwise return cluster.get("status", {}).get("state", cluster.get("state", "Unknown")), then replace the inline expression in cluster_tools.py (the current status = ... line) with a call to get_cluster_status(cluster). Ensure the helper is named and located near related utilities so callers can reuse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@assisted_service_mcp/src/api.py`:
- Around line 18-22: The module currently sets app = None when
settings.TRANSPORT == "stdio", which can cause AttributeError for imports that
assume app is an ASGI/HTTP app; update the module to either (a) add a clear
module-level docstring noting that app may be a sentinel when TRANSPORT ==
"stdio" and that callers must check settings.TRANSPORT before using app, or (b)
replace the bare None with a small sentinel object (e.g., _StdioSentinel or
DummyApp) that exposes minimal attributes or raises a clear, descriptive error
when accessed, and update any references in main.py to rely on
settings.TRANSPORT instead of assuming app is a real app; ensure the chosen
approach references the existing symbols app and settings.TRANSPORT so imports
won't accidentally call methods on None.
In `@assisted_service_mcp/src/main.py`:
- Around line 25-28: After initiating metrics with server.list_tools_sync() and
initiate_metrics(tool_names), add a check for stdio mode and emit a clear log
message stating that although metrics are initialized, the /metrics endpoint is
not exposed in stdio mode (or, alternatively, enable the endpoint if desired).
Locate the initialization block around server.list_tools_sync() /
initiate_metrics and add a conditional using whatever runtime flag or method
indicates stdio mode (e.g., an is_stdio or mode/config flag) to call
log.info("Metrics initialized for %d tools but /metrics endpoint is not exposed
in stdio mode", len(tool_names)) so users know metrics are not externally
accessible.
In `@assisted_service_mcp/src/service_client/assisted_service_api.py`:
- Around line 158-161: The substring check "clusters_mgmt" on self.inventory_url
is fragile; update the routing in the method that returns clusters (the block
that currently calls self.ocm_get_cluster(cluster_id)) to use a deterministic
signal instead: either read a new explicit configuration flag/API_TYPE (e.g.,
"ocm" vs "assisted-installer") and branch on that, or perform a stricter URL
match (e.g., parse self.inventory_url and compare the normalized path or
hostname rather than using "in"). Ensure the change replaces the fragile if
"clusters_mgmt" in self.inventory_url condition and still calls
ocm_get_cluster(cluster_id) when API_TYPE indicates OCM, otherwise falls back to
the existing assisted-installer flow.
- Around line 617-620: The OCM methods ocm_list_clusters and om_get_cluster
bypass the shared HTTP wrapper and thus miss API_CALL_LATENCY metrics; update
both functions to call the existing _api_call wrapper instead of directly
calling asyncio.to_thread(requests.get, ...). Pass the HTTP method ("GET"),
target url, headers and timeout into _api_call (and any expected response
handling), and adapt the call sites to use the returned response object from
_api_call so error handling and latency metrics remain consistent with other
Assisted Installer API calls.
In `@assisted_service_mcp/src/tools/cluster_tools.py`:
- Line 111: The complex inline status extraction should be moved into a small
helper function (e.g., get_cluster_status(cluster)) to improve readability:
implement get_cluster_status to return cluster.get("status") if it's a str,
otherwise return cluster.get("status", {}).get("state", cluster.get("state",
"Unknown")), then replace the inline expression in cluster_tools.py (the current
status = ... line) with a call to get_cluster_status(cluster). Ensure the helper
is named and located near related utilities so callers can reuse it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cf58c5e-e5a3-4a73-ab3b-22542ef1a89a
📒 Files selected for processing (5)
assisted_service_mcp/src/api.pyassisted_service_mcp/src/main.pyassisted_service_mcp/src/service_client/assisted_service_api.pyassisted_service_mcp/src/settings.pyassisted_service_mcp/src/tools/cluster_tools.py
|
ping |
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements