feat(modelsasservice): add deployment-based observability toggle - #3374
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a conditional telemetry configuration to ModelsAsService: Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Issues
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/23907043428 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go`:
- Around line 320-333: In configureIstioTelemetry, short-circuit when
observability is disabled before calling cluster.HasCRD: read the instance spec
(e.g., rr.Instance.Spec.Observability.Enabled or rr.Spec.Observability.Enabled)
and if it's false/nil log and return nil immediately, then only if observability
is enabled proceed to call cluster.HasCRD(gvk.Telemetry) and the existing flow;
this prevents transient CRD lookup failures from failing reconciles for disabled
observability.
- Around line 381-384: The Telemetry creation code that builds a selector using
gatewayName should pre-validate for selector conflicts instead of blindly
applying it; before appending the Telemetry with the selector map (the block
that sets "selector": map[string]any{"matchLabels":
{"gateway.networking.k8s.io/gateway-name": gatewayName}}) list existing Istio
Telemetry resources in the target namespace and compare their workload selectors
for overlap with the new selector; if an overlap/conflict is found, do not apply
the new Telemetry and surface a ModelsAsService status condition (use the
controller's status update / SetCondition logic on the ModelsAsService resource)
describing the conflicting Telemetry and why deployment is blocked, or
alternatively implement merging by owning a single controller-named Telemetry
resource (e.g., controller-owned Telemetry) and reconcile/merge required
selector/metric overrides into that single resource instead of creating separate
Telemetries per gateway.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e695a99a-6461-4478-b11f-ded90f940f9c
📒 Files selected for processing (7)
api/components/v1alpha1/modelsasservice_types.goapi/components/v1alpha1/zz_generated.deepcopy.gointernal/controller/components/modelsasservice/modelsasservice_controller.gointernal/controller/components/modelsasservice/modelsasservice_controller_actions.gointernal/controller/components/modelsasservice/modelsasservice_controller_actions_test.gointernal/controller/components/modelsasservice/modelsasservice_support.gointernal/controller/datasciencecluster/kubebuilder_rbac.go
| "selector": map[string]any{ | ||
| "matchLabels": map[string]any{ | ||
| "gateway.networking.k8s.io/gateway-name": gatewayName, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate the file
cd internal/controller/components/modelsasservice 2>/dev/null && pwd || echo "Directory not found"Repository: opendatahub-io/opendatahub-operator
Length of output: 144
🏁 Script executed:
# Find the file
find . -name "*modelsasservice_controller_actions.go" -type fRepository: opendatahub-io/opendatahub-operator
Length of output: 165
🏁 Script executed:
# Read the specific lines and surrounding context to understand the issue
cat -n internal/controller/components/modelsasservice/modelsasservice_controller_actions.go | sed -n '370,395p'Repository: opendatahub-io/opendatahub-operator
Length of output: 954
🏁 Script executed:
# Also read the second location (421-422) with context
cat -n internal/controller/components/modelsasservice/modelsasservice_controller_actions.go | sed -n '410,435p'Repository: opendatahub-io/opendatahub-operator
Length of output: 1219
🏁 Script executed:
# Search for Telemetry creation/appending logic to understand the pattern
rg -A 10 -B 5 "Telemetry" internal/controller/components/modelsasservice/modelsasservice_controller_actions.go | head -100Repository: opendatahub-io/opendatahub-operator
Length of output: 3665
🏁 Script executed:
# Check for conflict detection or existing Telemetry validation
rg "ConflictingTelemetry|IST0159|workload.*selector" internal/controller/components/modelsasservice/ || echo "No existing conflict detection found"Repository: opendatahub-io/opendatahub-operator
Length of output: 115
🏁 Script executed:
# Check if there's reconciliation logic that updates existing Telemetry resources
rg -B 5 -A 5 "IstioTelemetryName|rr.Resources" internal/controller/components/modelsasservice/ | head -50Repository: opendatahub-io/opendatahub-operator
Length of output: 5596
🏁 Script executed:
# Search for how resources are deployed/applied in reconciliation
rg -A 10 "Resources.*append\|reconciliation.*Telemetry" internal/controller/ | head -60Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if there's any logic that lists or filters existing Telemetry before creating
rg "Telemetry.*List\|GetTelemetry\|ListTelemetry" internal/controller/Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# See the complete function context for configureTelemetryPolicy to understand the flow
cat -n internal/controller/components/modelsasservice/modelsasservice_controller_actions.go | sed -n '300,370p'Repository: opendatahub-io/opendatahub-operator
Length of output: 3252
🏁 Script executed:
# Check how rr.Resources are deployed/applied in the reconciliation flow
rg -B 5 -A 10 "rr.Resources" internal/controller/components/modelsasservice/ | grep -A 10 -B 5 "apply\|Create\|Patch"Repository: opendatahub-io/opendatahub-operator
Length of output: 1949
🏁 Script executed:
# Search for how resources are deployed across the controller
rg -A 10 "ReconciliationRequest.*Resources\|apply.*Resources" internal/controller/ | head -80Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if there's any error handling or validation for resource deployment
rg "error.*Telemetry\|IST0159\|workload.*selector.*conflict" internal/Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Understand the reconciliation flow - check the main reconciler
find internal/controller -name "*.go" -type f | xargs grep -l "ReconciliationRequest" | head -3Repository: opendatahub-io/opendatahub-operator
Length of output: 307
🏁 Script executed:
# Look for how resources in rr.Resources are actually applied to the cluster
rg -B 3 -A 10 "\.Resources\s*=" internal/controller/modelsasservice_controller.go 2>/dev/null | head -60 || echo "File not found, searching differently..."Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Search more broadly for resource deployment
find internal -name "*controller.go" | xargs rg -A 15 "ReconciliationRequest" | grep -A 10 "Create\|Apply\|Deploy" | head -80Repository: opendatahub-io/opendatahub-operator
Length of output: 2153
🏁 Script executed:
# Find the main modelsasservice controller to understand resource deployment
cat internal/controller/components/modelsasservice/modelsasservice_controller.go | head -150Repository: opendatahub-io/opendatahub-operator
Length of output: 4843
🏁 Script executed:
# Search for how resources from rr.Resources are deployed in the base controller
find internal -name "*.go" -type f | xargs grep -l "rr.Resources" | head -5Repository: opendatahub-io/opendatahub-operator
Length of output: 389
🏁 Script executed:
# Look at the types.ReconciliationRequest to understand how Resources are used
rg -A 20 "type ReconciliationRequest" internal/Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if there's any logic that filters or deduplicates Telemetry resources
rg -B 5 -A 10 "Telemetry.*filter\|dedup\|merge\|conflict" internal/controller/Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Find where ReconciliationRequest is defined
find . -name "*.go" -type f | xargs grep -l "type ReconciliationRequest struct" | head -1Repository: opendatahub-io/opendatahub-operator
Length of output: 110
🏁 Script executed:
# Look at the deploy.NewAction to understand how rr.Resources are deployed
find . -name "*.go" -type f | xargs grep -l "deploy.NewAction\|DeployAction" | head -3Repository: opendatahub-io/opendatahub-operator
Length of output: 309
🏁 Script executed:
# Check the deploy action implementation
find . -path "*/actions/deploy/*" -name "*.go" | head -3Repository: opendatahub-io/opendatahub-operator
Length of output: 242
🏁 Script executed:
# Look at how resources are applied - search for Create/Patch patterns
rg -A 15 "func.*Apply\|func.*Create.*Resource" pkg/controller/actions/deploy/ 2>/dev/null | head -60Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if the deploy action handles resource conflicts or applies them as-is
cat pkg/controller/actions/deploy/action.go 2>/dev/null | head -100 || echo "File not found"Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Read the ReconciliationRequest type definition
cat -n pkg/controller/types/types.go | head -100Repository: opendatahub-io/opendatahub-operator
Length of output: 3785
🏁 Script executed:
# Read the deploy action implementation
cat -n pkg/controller/actions/deploy/action_deploy.go | head -150Repository: opendatahub-io/opendatahub-operator
Length of output: 4945
🏁 Script executed:
# Search for how resources are created/applied in deploy action
rg -B 5 -A 15 "Apply\|Create.*Unstructured\|client\.Create" pkg/controller/actions/deploy/Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if there's any validation or conflict detection for Telemetry
rg -B 5 -A 10 "ConflictingTelemetry\|IST0159\|overlapping.*selector" pkg/Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Continue reading the deploy action implementation
cat -n pkg/controller/actions/deploy/action_deploy.go | sed -n '150,250p'Repository: opendatahub-io/opendatahub-operator
Length of output: 4010
🏁 Script executed:
# Look for error handling or conflict detection in deploy
rg -A 20 "lookupErr\|Get.*current\|Create\|Patch" pkg/controller/actions/deploy/action_deploy.go | head -100Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check the full deploy action to see if resources are blindly applied or with conflict detection
wc -l pkg/controller/actions/deploy/action_deploy.goRepository: opendatahub-io/opendatahub-operator
Length of output: 129
🏁 Script executed:
# Read the rest of the deploy action
cat -n pkg/controller/actions/deploy/action_deploy.go | sed -n '150,300p'Repository: opendatahub-io/opendatahub-operator
Length of output: 5508
🏁 Script executed:
# Look at the full deploy function to see if there's any Telemetry-specific handling
cat -n pkg/controller/actions/deploy/action_deploy.go | sed -n '284,350p'Repository: opendatahub-io/opendatahub-operator
Length of output: 2755
🏁 Script executed:
# Check if there's multiple ModelsAsService instances or if only one can exist
rg "ModelsAsServiceInstanceName\|ModelsAsService" internal/controller/components/modelsasservice/ | grep -v "test.go" | grep -v "\.go-"Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Look for any documentation or comments about single instance
cat -n internal/controller/components/modelsasservice/modelsasservice_support.go | head -50Repository: opendatahub-io/opendatahub-operator
Length of output: 2422
🏁 Script executed:
# Check if the Telemetry name is derived from anything or if it's always the same
rg -B 5 "IstioTelemetryName.*=" internal/controller/components/modelsasservice/Repository: opendatahub-io/opendatahub-operator
Length of output: 830
🏁 Script executed:
# Check if ModelsAsServiceInstanceName is defined and if it's a singleton
rg "ModelsAsServiceInstanceName" api/components/v1alpha1/ | head -20Repository: opendatahub-io/opendatahub-operator
Length of output: 200
🏁 Script executed:
# Look for how ModelsAsService instances are managed - can there be multiple?
rg -B 5 -A 5 "ModelsAsService.*singleton\|ModelsAsService.*instance" internal/ api/Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check the CRD definition to understand cardinality constraints
find . -path "*/api/*" -name "*modelsasservice*.go" | head -5Repository: opendatahub-io/opendatahub-operator
Length of output: 129
🏁 Script executed:
# Read the ModelsAsService types definition
cat -n api/components/v1alpha1/modelsasservice_types.go | head -100Repository: opendatahub-io/opendatahub-operator
Length of output: 5080
🏁 Script executed:
# Look for any validation or uniqueness constraints
rg "kubebuilder:validation\|webhook\|unique\|singleton" api/components/v1alpha1/modelsasservice_types.goRepository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if there's any constraint that only one ModelsAsService can exist
rg -B 5 -A 5 "ModelsAsServiceInstanceName" api/components/v1alpha1/modelsasservice_types.goRepository: opendatahub-io/opendatahub-operator
Length of output: 438
🏁 Script executed:
# Check if there's error handling for Istio validation failures
rg "IST0159\|ConflictingTelemetry\|validation.*error\|admission" internal/controller/components/modelsasservice/ || echo "No error handling found"Repository: opendatahub-io/opendatahub-operator
Length of output: 102
🏁 Script executed:
# Look for any tests that demonstrate the conflict scenario
rg -l "Telemetry.*conflict\|overlap" internal/controller/components/modelsasservice/ || echo "No conflict tests found"Repository: opendatahub-io/opendatahub-operator
Length of output: 102
🏁 Script executed:
# Check if there are any pre-deployment validations or checks
rg -B 5 -A 10 "configureIstioTelemetry\|Telemetry.*validation\|Get.*Telemetry" internal/controller/components/modelsasservice/modelsasservice_controller_actions.goRepository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Let me verify the complete picture - check if GatewayRef can vary or if it's also fixed
rg "GatewayRef" api/components/v1alpha1/modelsasservice_types.go -A 5 | head -20Repository: opendatahub-io/opendatahub-operator
Length of output: 911
🏁 Script executed:
# Check if there's any scenario where multiple Telemetry could be created from the same ModelsAsService
rg -B 10 -A 10 "configureIstioTelemetry\|IstioTelemetryName" internal/controller/components/modelsasservice/modelsasservice_controller.goRepository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Verify the flow - is configureTelemetryPolicy different from configureIstioTelemetry?
cat -n internal/controller/components/modelsasservice/modelsasservice_controller_actions.go | sed -n '240,280p'Repository: opendatahub-io/opendatahub-operator
Length of output: 1849
Pre-validate Telemetry selector conflicts before deployment instead of relying on Istio admission rejection.
The code creates an Istio Telemetry resource with a gateway-specific selector and blindly appends it without checking for overlapping workload selectors in the gateway namespace. Istio treats overlapping selectors in the same namespace as IST0159 ConflictingTelemetryWorkloadSelectors and will reject the configuration. While the ModelsAsService CRD enforces a singleton instance (name validation), this doesn't prevent conflicts with Telemetry from other controllers or user-created resources in the gateway namespace. The deployment will fail without a clear error signal to the user.
Implement pre-deployment validation: either query existing Telemetry resources in the target namespace for selector conflicts and surface a condition on the ModelsAsService status, or consolidate metrics overrides into a single controller-owned Telemetry resource merged across reconciliations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go`
around lines 381 - 384, The Telemetry creation code that builds a selector using
gatewayName should pre-validate for selector conflicts instead of blindly
applying it; before appending the Telemetry with the selector map (the block
that sets "selector": map[string]any{"matchLabels":
{"gateway.networking.k8s.io/gateway-name": gatewayName}}) list existing Istio
Telemetry resources in the target namespace and compare their workload selectors
for overlap with the new selector; if an overlap/conflict is found, do not apply
the new Telemetry and surface a ModelsAsService status condition (use the
controller's status update / SetCondition logic on the ModelsAsService resource)
describing the conflicting Telemetry and why deployment is blocked, or
alternatively implement merging by owning a single controller-named Telemetry
resource (e.g., controller-owned Telemetry) and reconcile/merge required
selector/metric overrides into that single resource instead of creating separate
Telemetries per gateway.
58a85d3 to
b808976
Compare
jland-redhat
left a comment
jland-redhat
left a comment
There was a problem hiding this comment.
Lets validate with Lindani but I think we don't want this to be opt-in
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/controller/components/modelsasservice/modelsasservice_controller_actions.go (1)
380-383:⚠️ Potential issue | 🟠 MajorSelector-conflict handling is missing before Telemetry append (IST0159 failure path).
Line 380–383 assumes singleton/name/ownerRef mitigate conflicts, but they do not prevent overlap with pre-existing Telemetry resources in the same namespace. Admission rejection here can cause repeated reconcile failures without a clear, actionable status on the ModelsAsService CR. Pre-validate selector overlap and set a status condition before appending the resource.
#!/bin/bash # Verify there is no selector-conflict prevalidation/status condition path # before appending Istio Telemetry resources. rg -n -C3 'configureIstioTelemetryCore|rr\.Resources = append\(rr\.Resources, \*istioTelemetry\)|IST0159|ConflictingTelemetry|SetCondition|Condition' \ internal/controller/components/modelsasservice/modelsasservice_controller_actions.go \ internal/controller/components/modelsasservice/modelsasservice_controller.goAs per coding guidelines,
**: REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code) 3. Bug-prone patterns and error handling gaps 4. Performance problems.Also applies to: 436-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go` around lines 380 - 383, Pre-validate selector overlap before appending the Istio Telemetry resource: inside the code path that calls configureIstioTelemetryCore and before rr.Resources = append(rr.Resources, *istioTelemetry) (and the similar block around lines 436–437), query existing Telemetry resources in the target namespace and check selectors for conflicts; if a conflict is detected, do not append the resource and instead set a status condition on the ModelsAsService CR (use your SetCondition/Status update helpers, e.g. create a ConflictingTelemetry condition with helpful message and reason IST0159) so reconciliation surfaces a clear, actionable error and avoids admission rejections and retry storms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go`:
- Around line 415-417: The subscription header is being used directly as a
metric label ("subscription": request.headers["x-maas-subscription"]) which
permits header spoofing and high-cardinality data; modify the upsert logic in
the ModelsasService controller action to not trust raw
request.headers["x-maas-subscription"] — instead retrieve the subscription from
a validated auth/context source (e.g., the authenticated user/session object) or
validate/normalize the header against a strict allowlist of expected
subscription IDs before including it, and if it fails validation fall back to a
safe low-cardinality value (e.g., "unknown" or omit the label); ensure this
change is applied where the metric map with key "subscription" is built so
high-cardinality labels are never emitted.
---
Duplicate comments:
In
`@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go`:
- Around line 380-383: Pre-validate selector overlap before appending the Istio
Telemetry resource: inside the code path that calls configureIstioTelemetryCore
and before rr.Resources = append(rr.Resources, *istioTelemetry) (and the similar
block around lines 436–437), query existing Telemetry resources in the target
namespace and check selectors for conflicts; if a conflict is detected, do not
append the resource and instead set a status condition on the ModelsAsService CR
(use your SetCondition/Status update helpers, e.g. create a ConflictingTelemetry
condition with helpful message and reason IST0159) so reconciliation surfaces a
clear, actionable error and avoids admission rejections and retry storms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 85ac3a70-9e5b-4ed9-9112-ce5317cebfdb
📒 Files selected for processing (1)
internal/controller/components/modelsasservice/modelsasservice_controller_actions.go
853e0a9 to
22b27e6
Compare
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/components/v1alpha1/modelsasservice_types.go (1)
76-82:⚠️ Potential issue | 🟠 Major
enabledbecomes opt-out for existingspec.telemetryusers.Line 80 defaults the new field to
true.TelemetryConfigalready existed before this PR, so any CR that already has aspec.telemetryblock but omitsenabledcan start reconciling the new IstioTelemetryresource once CRD defaulting applies. For a technical-preview path, keepnil/falseas disabled and require an explicit opt-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/components/v1alpha1/modelsasservice_types.go` around lines 76 - 82, The new Enabled field on TelemetryConfig defaults to true which causes existing CRs that omit spec.telemetry.enabled to be treated as opt-out; remove the "+kubebuilder:default=true" annotation (or change it so no CRD default is applied) on the Enabled *bool field in TelemetryConfig (symbol: Enabled in modelsasservice_types.go) so that omitted values remain nil, and update any controller logic that reads TelemetryConfig.Enabled (e.g., any reconcile code) to treat only explicit true as enabled (nil/false = disabled) to enforce explicit opt-in for the technical-preview path.internal/controller/components/modelsasservice/modelsasservice_controller_actions.go (1)
388-406:⚠️ Potential issue | 🟠 MajorThe selector-conflict mitigation here does not cover external
Telemetryobjects.The singleton CR, fixed resource name, and owner refs only deduplicate this controller’s own object. Istio still rejects overlapping workload selectors from any other
telemetry.istio.io/v1Telemetryin the gateway namespace (ConflictingTelemetryWorkloadSelectors/ IST0159), and this path does no preflight or targeted status reporting before append. Check for conflicting selectors first, or surface a condition that points to the conflicting resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go` around lines 388 - 406, The current creation of istioTelemetry (variable istioTelemetry, using IstioTelemetryName, gatewayNamespace, gatewayName) can still conflict with other telemetry. Before applying or patching that unstructured Telemetry, list existing telemetry.istio.io/v1 Telemetry objects in gatewayNamespace, inspect their spec.selector.matchLabels for overlap with the selector you plan to use (gateway.networking.k8s.io/gateway-name -> gatewayName), and if any conflict is found do not apply the new Telemetry; instead set a status condition on the ModelsAsService CR (e.g., TelemetrySelectorConflictCondition) describing the conflicting Telemetry name(s) and return an error/result that surfaces the conflict; if no conflicts, proceed to create/patch istioTelemetry as currently implemented. Ensure you reference the istioTelemetry object and IstioTelemetryName when implementing the check so the same selector logic is used for the comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go`:
- Around line 320-343: The telemetry enable check currently exists in
configureIstioTelemetry but configureTelemetryPolicy is still invoked
unconditionally from the reconciler, causing usage-telemetry to be reconciled
even when maas.Spec.Telemetry.Enabled is false; update the reconcile flow so
both configureIstioTelemetry and configureTelemetryPolicy are gated by the same
helper (e.g., add or reuse a function like isTelemetryEnabled(ctx, rr or maas)
that checks maas.Spec.Telemetry != nil and Enabled != nil && *Enabled) and call
that helper before invoking configureTelemetryPolicy and configureIstioTelemetry
(or add the same nil/Enabled check at the top of configureTelemetryPolicy) so
neither path runs or performs CRD lookups when telemetry is disabled.
---
Duplicate comments:
In `@api/components/v1alpha1/modelsasservice_types.go`:
- Around line 76-82: The new Enabled field on TelemetryConfig defaults to true
which causes existing CRs that omit spec.telemetry.enabled to be treated as
opt-out; remove the "+kubebuilder:default=true" annotation (or change it so no
CRD default is applied) on the Enabled *bool field in TelemetryConfig (symbol:
Enabled in modelsasservice_types.go) so that omitted values remain nil, and
update any controller logic that reads TelemetryConfig.Enabled (e.g., any
reconcile code) to treat only explicit true as enabled (nil/false = disabled) to
enforce explicit opt-in for the technical-preview path.
In
`@internal/controller/components/modelsasservice/modelsasservice_controller_actions.go`:
- Around line 388-406: The current creation of istioTelemetry (variable
istioTelemetry, using IstioTelemetryName, gatewayNamespace, gatewayName) can
still conflict with other telemetry. Before applying or patching that
unstructured Telemetry, list existing telemetry.istio.io/v1 Telemetry objects in
gatewayNamespace, inspect their spec.selector.matchLabels for overlap with the
selector you plan to use (gateway.networking.k8s.io/gateway-name ->
gatewayName), and if any conflict is found do not apply the new Telemetry;
instead set a status condition on the ModelsAsService CR (e.g.,
TelemetrySelectorConflictCondition) describing the conflicting Telemetry name(s)
and return an error/result that surfaces the conflict; if no conflicts, proceed
to create/patch istioTelemetry as currently implemented. Ensure you reference
the istioTelemetry object and IstioTelemetryName when implementing the check so
the same selector logic is used for the comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 289d6ac5-992e-4beb-abf1-46decd66753a
📒 Files selected for processing (5)
api/components/v1alpha1/modelsasservice_types.goapi/components/v1alpha1/zz_generated.deepcopy.godocs/api-overview.mdinternal/controller/components/modelsasservice/modelsasservice_controller_actions.gointernal/controller/components/modelsasservice/modelsasservice_controller_actions_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kahowell 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3374 +/- ##
==========================================
+ Coverage 50.94% 51.11% +0.16%
==========================================
Files 199 199
Lines 14727 14817 +90
==========================================
+ Hits 7502 7573 +71
- Misses 6479 6497 +18
- Partials 746 747 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6547b0a to
fcb11ec
Compare
|
/lgtm |
|
/hold Revision fcb11ec was retested 3 times: holding |
|
/retest |
1 similar comment
|
/retest |
|
/lgtm |
65ac0b4 to
ae60516
Compare
|
/lgtm |
|
/unhold |
115ed0a
into
opendatahub-io:main
6 participants
Description
Add deployment-based observability support to the ModelsAsService component, enabling per-subscription latency tracking via Istio Telemetry resources.
Changes:
Feature behavior:
When spec.observability.enabled: true is set in the ModelsAsService CR, the operator creates an Istio Telemetry resource that adds a subscription label to
istio_request_duration_milliseconds_bucketmetrics, extracted from theX-MaaS-Subscriptionheader injected by AuthPolicy. This enables P50/P95/P99 latency tracking per subscription in Grafana dashboards.RHOAIENG-56531
How Has This Been Tested?
Screenshot or short clip
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification
This PR adds a new optional, disabled-by-default technical preview feature. The feature:
Summary by CodeRabbit
New Features
Tests
Chores
Documentation