OLS-3726: Replace RHOKP sidecar with standalone HTTPS Deployment - #1896
Conversation
openshift-ci
Bot
added
the
needs-rebase
📝 WalkthroughWalkthroughThe PR adds standalone RHOKP resource reconciliation and deployment lifecycle management, integrates RHOKP CA trust and client-secret refresh into the app server, gates MCP/RHOKP reconciliation by configuration, and replaces ChangesStandalone RHOKP integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WatchedSecret
participant Watchers
participant RHOKP
participant AppServer
participant AgenticConfiguration
WatchedSecret->>Watchers: TLS data changes
Watchers->>RHOKP: Restart deployment
Watchers->>AppServer: Refresh client CA Secrets and restart
Watchers->>AgenticConfiguration: Touch reload annotation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/controller/olsconfig_controller.go (1)
467-517: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd top-level RHOKP gating coverage.
Add co-located tests for enabled→disabled cleanup and the resulting
RHOKPReady=False, Reason=Disabledstatus condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/olsconfig_controller.go` around lines 467 - 517, Add co-located controller tests covering the RHOKP top-level gating path: verify that changing ByokRAGOnly from enabled to disabled removes or cleans up the RHOKP deployment, and assert the resulting RHOKPReady condition is False with Reason set to Disabled. Reuse the existing OLSConfig reconciliation test helpers and fixtures.Sources: Coding guidelines, Path instructions
.ai/spec/what/app-server.md (2)
8-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale RHOKP sidecar description.
Line 8 still says the app-server has a RHOKP sidecar, contradicting the new standalone Deployment description. Update the composition rule to describe only the remaining sidecars.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.ai/spec/what/app-server.md around lines 8 - 12, Update the deployment composition statement in the app-server specification to list only the data collector as a possible sidecar; remove the stale RHOKP sidecar reference while preserving the primary API container and standalone RHOKP Deployment descriptions.
107-112: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse
lightspeed-agentic-configurationfor the agentic handoff ConfigMap.
.ai/spec/what/agentic-sandbox-profile.md,tls.md,rhokp.md, andsystem-overview.mdidentifylightspeed-agentic-configurationas the canonical agentic handoff ConfigMap. Either rename the app-server section and OLS-3572 reference to that name or provide implementation evidence thatlightspeed-sandbox-configis intentionally separate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.ai/spec/what/app-server.md around lines 107 - 112, Update the app-server specification to use the canonical lightspeed-agentic-configuration ConfigMap name instead of lightspeed-sandbox-config, including the ConfigMap description and any OLS-3572 reference. Keep the documented keys and contents unchanged unless implementation evidence requires documenting these as separate ConfigMaps..ai/spec/how/reconciliation.md (1)
20-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the documented Phase 1 order to the controller.
The supplied
reconcileIndependentResources()appends Console before PostgreSQL, while this section claims PostgreSQL is first. Either update the diagram or change the controller; the documented order should be authoritative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.ai/spec/how/reconciliation.md around lines 20 - 21, Align the Phase 1 sequence documented around reconcileIndependentResources() with the controller’s actual order: ensure PostgreSQL is listed before Console, either by updating the diagram or changing the controller, while keeping the documented order authoritative.internal/controller/appserver/deployment.go (1)
556-559: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comment: RHOKP sidecar no longer added here.
🧹 Proposed fix
- // Add additional containers in a consistent order: - // 1. Data collector container (if enabled) - // 2. RHOKP Solr sidecar (if Solr hybrid RAG is configured) - + // Add the data collector container (if enabled). + // RHOKP now runs as a standalone Deployment, not a sidecar.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/deployment.go` around lines 556 - 559, Update the comment near the additional-container assembly to remove the RHOKP Solr sidecar entry, leaving only the data collector container description and any ordering information that still reflects the implementation.
🧹 Nitpick comments (8)
cmd/main.go (1)
441-445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale
ACTIVE_BACKENDguidance.The watcher mapping now uses explicit targets; this comment can encourage reintroducing the retired pseudo-target.
Proposed update
- // Only list secrets here that need to restart specific deployments beyond the active backend. + // List explicit restart targets for operator-managed annotated secrets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/main.go` around lines 441 - 445, Update the comment documenting AnnotatedSecretMapping to remove any stale ACTIVE_BACKEND guidance or references to the retired pseudo-target, while retaining the explanation of explicit secret-to-deployment mappings and watcher behavior.Source: Path instructions
internal/controller/appserver/assets_test.go (2)
1346-1352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
By("Disabling introspection")block now asserts almost nothing.After dropping the annotation check, this step only re-verifies container count, which is unchanged by introspection. Either assert the MCP CA volume is absent (as
deployment_test.goLines 175-177 does) or drop the block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/assets_test.go` around lines 1346 - 1352, Update the “Disabling introspection” block in the GenerateOLSDeployment test to verify the MCP CA volume is absent, matching the existing assertion pattern in the related deployment test; otherwise remove the block instead of retaining the unchanged container-count assertion.
2057-2061: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCount-based helper names are now stale.
get7RequiredVolumeMounts/get7RequiredVolumesreturn 9 entries after the RHOKP additions (and theget8/get9/get10chain shifts with them). Intent-based names (e.g.baseRequiredVolumes,withDataCollectorVolumes) would stop this drift and avoid hard-coded counts like Lines 815/838/861.Also applies to: 2167-2181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/assets_test.go` around lines 2057 - 2061, The volume helper names use stale count-based identifiers and hard-coded count progression. Rename get7RequiredVolumeMounts/get7RequiredVolumes and the related get8/get9/get10 helpers to intent-based names reflecting the volume composition, then update all callers and affected test references, including the checks around the RHOKP additions, to remove hard-coded counts.internal/controller/appserver/deployment_test.go (2)
729-762: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the RHOKP CA volume under
byokRAGOnly: true.This test already sets
ByokRAGOnly = true, which is exactly the branch that should suppress the newrhokp-cavolume/mount (deployment.goLines 366-383, 413-419) and theextra_caentry. Asserting absence here is a one-liner and locks in the gating both directions.💚 Suggested additional assertions
for i := range dep.Spec.Template.Spec.Containers { Expect(dep.Spec.Template.Spec.Containers[i].Name).NotTo(Equal(utils.RHOOKPContainerName)) } + + for _, v := range dep.Spec.Template.Spec.Volumes { + Expect(v.Name).NotTo(Equal(utils.AppRHOKPCACertVolumeName)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/deployment_test.go` around lines 729 - 762, Add assertions to the existing “should not include RHOKP sidecar even when byokRAGOnly is true” test to verify the generated deployment has no rhokp-ca volume, no corresponding volume mount, and no extra_ca entry. Use the deployment returned by GenerateOLSDeployment and the existing RHOKP CA identifiers, preserving the current sidecar and environment-variable assertions.
553-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated hard-coded
extra_caordering literal across three test files. The same long ordered path string is asserted in three places, so any change to theExtraCAsappend order ininternal/controller/appserver/assets.go(Lines 528-563) requires three synchronized edits — and a missed one silently weakens coverage rather than failing loudly.
internal/controller/appserver/deployment_test.go#L553-L553: replace the substring match with an unmarshal ofOLSConfig.ExtraCAscompared against a slice built fromutils.App*CACertDir/utils.App*CACertFileconstants.internal/controller/appserver/assets_test.go#L1781-L1781: use the same shared expected-ExtraCAshelper instead of the inline literal.internal/controller/appserver/reconciler_test.go#L662-L662: use the same shared expected-ExtraCAshelper instead of the inline literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/deployment_test.go` at line 553, Replace the duplicated hard-coded extra_ca assertions with one shared expected-ExtraCAs helper that builds the slice from the utils.App*CACertDir and utils.App*CACertFile constants, unmarshals OLSConfig.ExtraCAs, and compares the result. Update deployment_test.go:553 to perform this unmarshal and comparison, and update assets_test.go:1781 and reconciler_test.go:662 to reuse the shared helper.internal/controller/appserver/assets.go (2)
1112-1116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGeneric helper keyed on an OTEL-specific constant.
generateClientCAis now shared by OTEL/MCP/RHOKP but readsutils.AppOtelCollectorCACertFileas the source key of the cluster service-ca ConfigMap. The value is right, but the name misleads readers into thinking it's OTEL-specific. A neutral constant (e.g.ServiceCACertKey) — or a per-entrySourceKeyfield onclientCAConfig— would make the contract explicit.As per coding guidelines: "Use naming patterns such as ... constants like
OLSConfigCmName".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/assets.go` around lines 1112 - 1116, Update the shared generateClientCA configuration to use a neutral service-CA key constant or an explicit clientCAConfig.SourceKey field instead of utils.AppOtelCollectorCACertFile. Ensure the OTEL, MCP, and RHOKP callers all resolve the same ConfigMap entry through that neutral contract, and use it in the missing-or-empty error message.Source: Coding guidelines
1065-1101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMislabelled error constants in the table.
Two entries reuse a "get" message for a different operation, which will produce misleading errors:
- OTEL
ErrDelete: utils.ErrGetAgenticOtelCASecret(delete path reports a get failure; currently unreachable since OTEL is always enabled).- RHOKP
ErrSource: utils.ErrGetAgenticRHOKPCASecret—ErrSourceis used when reading the service-ca ConfigMap, so the message should mirrorErrGetAgenticOtelCASourceConfigMap.Consider adding
ErrDeleteAgenticOtelCASecretandErrGetAgenticRHOKPCASourceConfigMapininternal/controller/utils/errors.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/assets.go` around lines 1065 - 1101, Correct the error mappings in clientCASecrets: set the OTEL entry’s ErrDelete to a dedicated ErrDeleteAgenticOtelCASecret constant, and set the RHOKP entry’s ErrSource to ErrGetAgenticRHOKPCASourceConfigMap. Add both missing constants in errors.go with operation-specific messages matching the existing error conventions.internal/controller/rhokp/deployment.go (1)
47-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTwo inconsistent naming prefixes ("RHOKP" vs "RHOOKP") for the same component, mixed throughout the new package. This PR introduces
RHOKP-prefixed constants (RHOKPDeploymentName,RHOKPServiceName,RHOKPCertsSecretName, etc.) for the new standalone Deployment/Service/NetworkPolicy, while the same new files continue to reference pre-existingRHOOKP-prefixed constants (RHOOKPImageHTTPSPort,RHOOKPContainerName,RHOOKPImageDefault, etc.) for the container/image/probe settings — both referring to the identical RHOKP feature. This breaks the<Component><Resource>Namenaming convention's intent of a single consistent component prefix and will likely confuse future contributors to this new package.
internal/controller/rhokp/deployment.go#L47-L184: confirm whether "RHOKP" or "RHOOKP" is the intended canonical spelling, then align the constants used inGenerateDeployment(mixes both prefixes for container name/image/probes vs. TLS/Solr volume/deployment name) to one prefix.internal/controller/utils/constants.go#L706-L710: alignRHOKPServiceURL's use ofRHOOKPImageHTTPSPortwith whichever prefix is chosen.internal/controller/rhokp/assets.go#L29-L97: alignGenerateService/GenerateNetworkPolicy's use ofRHOOKPImageHTTPSPortwith the chosen prefix.internal/controller/rhokp/reconciler_test.go#L97-L107: update theRHOOKPImageDefaultassertion if the canonical constant name changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/rhokp/deployment.go` around lines 47 - 184, Standardize the RHOKP component prefix by choosing one canonical spelling and renaming or updating all related constants and references consistently. In internal/controller/rhokp/deployment.go lines 47-184, internal/controller/utils/constants.go lines 706-710, and internal/controller/rhokp/assets.go lines 29-97, align container, image, probe, port, service URL, service, and network-policy references; update internal/controller/rhokp/reconciler_test.go lines 97-107 to assert the canonical image constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/how/project-structure.md:
- Around line 117-118: Update the affected documentation to distinguish
deployment restart callbacks from ConfigMap touches: in
.ai/spec/how/project-structure.md lines 117-118, document deployment callbacks
separately and represent lightspeed-agentic-configuration as a
TouchAgenticConfiguration callback without deployment force-reload semantics; in
.ai/spec/what/resource-lifecycle.md lines 33-35, likewise identify
lightspeed-agentic-configuration separately from deployment targets and
force-reload behavior.
In @.ai/spec/how/reconciliation.md:
- Line 35: Update the reconciliation documentation’s CA publication description
to state that app-server publishes OTEL CA unconditionally, MCP CA only when
introspection is enabled, and RHOKP CA only when OKP is enabled; remove the
implication that all three are always published.
- Line 31: Update the reconciliation documentation entry around
rhokp.ReconcileResources to name the target explicitly as app-server or
lightspeed-app-server instead of “active backend.” Remove references to the
ACTIVE_BACKEND pseudo-target and reflect that the controller directly invokes
appserver.ReconcileAppServerDeployment().
In @.ai/spec/what/agentic-sandbox-profile.md:
- Around line 50-54: Update the shared serving-certificate rotation
documentation in .ai/spec/what/agentic-sandbox-profile.md:50-54 to describe the
handoff ConfigMap touch only as part of RestartAppServer, removing the separate
final TouchAgenticConfiguration from each watcher sequence while preserving the
RestartAppServer order and fail-closed behavior. Update the corresponding
rotation descriptions in .ai/spec/what/tls.md:37-39 and
.ai/spec/what/rhokp.md:57-60 to reference the shared RestartAppServer flow and
remove wording that implies an additional handoff-ConfigMap touch.
In @.ai/spec/what/app-server.md:
- Line 12: Update the app-server documentation to remove the stale
ols.openshift.io/mcp-server-ca-configmap-hash change-detection rule and describe
the table-driven Secret refresh/update behavior introduced by the cutover. Keep
the guidance consistent with the current app-server deployment generation and
identify the relevant Secret-driven update behavior rather than MCP CA hash
tracking.
In @.ai/spec/what/crd-api.md:
- Line 546: Update the planned-status documentation so OLS-3697 remains marked
DONE only for the completed RHOKP standalone conversion in
.ai/spec/what/crd-api.md:546-546. In .ai/spec/what/rhokp.md:82-82, mark the
ServiceMonitor work as deferred and keep it consistent with the existing
Monitoring section; update any related planned-status wording consistently
without changing the completed conversion status.
In `@AGENTS.md`:
- Around line 68-69: Align the RHOKP disabled-status contract between AGENTS.md
and rhokp.md by choosing one behavior: either emit RHOKPReady=True with
Reason=NotConfigured when byokRAGOnly is true, or emit no RHOKP condition.
Update the conflicting documentation consistently, including the
ReconcileDeployment behavior description.
In `@internal/controller/appserver/deployment.go`:
- Around line 59-64: Update the SSL_CERT_FILE value in the deployment
environment configuration to reference the certificate bundle filename actually
generated by lightspeed-service or an operator-managed file guaranteed to exist
in the configured certificate volume. Remove the hardcoded ols.pem assumption
while preserving the existing OLSAppCertsMountRoot and CertBundleVolumeName
mount path.
In `@internal/controller/olsconfig_controller.go`:
- Around line 331-347: Update the cleanup error assignments in the MCP server
and RHOKP branches of the OLSConfig reconciliation flow to use the corresponding
shared utils.Err... constants from errors.go. Wrap each underlying error with
the required fmt.Errorf("%s: %w", utils.Err..., err) format, preserving the
existing resourceFailures keys and cleanup behavior.
In `@internal/controller/olsconfig_helpers.go`:
- Around line 472-479: Add regression tests for RHOKP TLS rotation at
internal/controller/olsconfig_helpers.go:472-479 and
internal/controller/watchers/watchers.go:382-389. In the helper tests, verify
ByokRAGOnly toggles RHOKPTLSWatchEnabled and suppresses or permits the static
RHOKP TLS Secret watch; in the watcher tests, verify RHOKP certificate rotation
restarts lightspeed-rhokp and updates the agentic configuration ConfigMap.
In `@internal/controller/rhokp/reconciler.go`:
- Around line 45-49: Update reconcileNetworkPolicy to wrap the error returned by
GenerateNetworkPolicy with utils.ErrGenerateRHOKPNetworkPolicy, matching the
existing error-wrapping behavior in reconcileService and reconcileDeployment;
preserve the current successful path and returned error context.
---
Outside diff comments:
In @.ai/spec/how/reconciliation.md:
- Around line 20-21: Align the Phase 1 sequence documented around
reconcileIndependentResources() with the controller’s actual order: ensure
PostgreSQL is listed before Console, either by updating the diagram or changing
the controller, while keeping the documented order authoritative.
In @.ai/spec/what/app-server.md:
- Around line 8-12: Update the deployment composition statement in the
app-server specification to list only the data collector as a possible sidecar;
remove the stale RHOKP sidecar reference while preserving the primary API
container and standalone RHOKP Deployment descriptions.
- Around line 107-112: Update the app-server specification to use the canonical
lightspeed-agentic-configuration ConfigMap name instead of
lightspeed-sandbox-config, including the ConfigMap description and any OLS-3572
reference. Keep the documented keys and contents unchanged unless implementation
evidence requires documenting these as separate ConfigMaps.
In `@internal/controller/appserver/deployment.go`:
- Around line 556-559: Update the comment near the additional-container assembly
to remove the RHOKP Solr sidecar entry, leaving only the data collector
container description and any ordering information that still reflects the
implementation.
In `@internal/controller/olsconfig_controller.go`:
- Around line 467-517: Add co-located controller tests covering the RHOKP
top-level gating path: verify that changing ByokRAGOnly from enabled to disabled
removes or cleans up the RHOKP deployment, and assert the resulting RHOKPReady
condition is False with Reason set to Disabled. Reuse the existing OLSConfig
reconciliation test helpers and fixtures.
---
Nitpick comments:
In `@cmd/main.go`:
- Around line 441-445: Update the comment documenting AnnotatedSecretMapping to
remove any stale ACTIVE_BACKEND guidance or references to the retired
pseudo-target, while retaining the explanation of explicit secret-to-deployment
mappings and watcher behavior.
In `@internal/controller/appserver/assets_test.go`:
- Around line 1346-1352: Update the “Disabling introspection” block in the
GenerateOLSDeployment test to verify the MCP CA volume is absent, matching the
existing assertion pattern in the related deployment test; otherwise remove the
block instead of retaining the unchanged container-count assertion.
- Around line 2057-2061: The volume helper names use stale count-based
identifiers and hard-coded count progression. Rename
get7RequiredVolumeMounts/get7RequiredVolumes and the related get8/get9/get10
helpers to intent-based names reflecting the volume composition, then update all
callers and affected test references, including the checks around the RHOKP
additions, to remove hard-coded counts.
In `@internal/controller/appserver/assets.go`:
- Around line 1112-1116: Update the shared generateClientCA configuration to use
a neutral service-CA key constant or an explicit clientCAConfig.SourceKey field
instead of utils.AppOtelCollectorCACertFile. Ensure the OTEL, MCP, and RHOKP
callers all resolve the same ConfigMap entry through that neutral contract, and
use it in the missing-or-empty error message.
- Around line 1065-1101: Correct the error mappings in clientCASecrets: set the
OTEL entry’s ErrDelete to a dedicated ErrDeleteAgenticOtelCASecret constant, and
set the RHOKP entry’s ErrSource to ErrGetAgenticRHOKPCASourceConfigMap. Add both
missing constants in errors.go with operation-specific messages matching the
existing error conventions.
In `@internal/controller/appserver/deployment_test.go`:
- Around line 729-762: Add assertions to the existing “should not include RHOKP
sidecar even when byokRAGOnly is true” test to verify the generated deployment
has no rhokp-ca volume, no corresponding volume mount, and no extra_ca entry.
Use the deployment returned by GenerateOLSDeployment and the existing RHOKP CA
identifiers, preserving the current sidecar and environment-variable assertions.
- Line 553: Replace the duplicated hard-coded extra_ca assertions with one
shared expected-ExtraCAs helper that builds the slice from the
utils.App*CACertDir and utils.App*CACertFile constants, unmarshals
OLSConfig.ExtraCAs, and compares the result. Update deployment_test.go:553 to
perform this unmarshal and comparison, and update assets_test.go:1781 and
reconciler_test.go:662 to reuse the shared helper.
In `@internal/controller/rhokp/deployment.go`:
- Around line 47-184: Standardize the RHOKP component prefix by choosing one
canonical spelling and renaming or updating all related constants and references
consistently. In internal/controller/rhokp/deployment.go lines 47-184,
internal/controller/utils/constants.go lines 706-710, and
internal/controller/rhokp/assets.go lines 29-97, align container, image, probe,
port, service URL, service, and network-policy references; update
internal/controller/rhokp/reconciler_test.go lines 97-107 to assert the
canonical image constant.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93842531-2f83-4cff-8fe4-cb5f29b22068
⛔ Files ignored due to path filters (1)
config/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (37)
.ai/spec/how/project-structure.md.ai/spec/how/reconciliation.md.ai/spec/what/agentic-sandbox-profile.md.ai/spec/what/app-server.md.ai/spec/what/crd-api.md.ai/spec/what/ocpmcp.md.ai/spec/what/reconciliation.md.ai/spec/what/resource-lifecycle.md.ai/spec/what/rhokp.md.ai/spec/what/tls.mdAGENTS.mdapi/v1alpha1/olsconfig_types.gocmd/main.goconfig/default/deployment-patch.yamlinternal/controller/agenticintegration/assets.gointernal/controller/agenticintegration/assets_test.gointernal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/appserver/deployment.gointernal/controller/appserver/deployment_test.gointernal/controller/appserver/reconciler.gointernal/controller/appserver/reconciler_test.gointernal/controller/ocpmcp/reconciler.gointernal/controller/ocpmcp/reconciler_test.gointernal/controller/olsconfig_controller.gointernal/controller/olsconfig_helpers.gointernal/controller/olsconfig_helpers_test.gointernal/controller/rhokp/assets.gointernal/controller/rhokp/deployment.gointernal/controller/rhokp/reconciler.gointernal/controller/rhokp/reconciler_test.gointernal/controller/rhokp/suite_test.gointernal/controller/utils/constants.gointernal/controller/utils/errors.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.go
💤 Files with no reviewable changes (2)
- config/default/deployment-patch.yaml
- internal/controller/ocpmcp/reconciler.go
| | |-- otelcollector.ReconcileOtelCollectorResources() | ||
| | |-- ocpmcp.ReconcileResources() | ||
| | | (when introspectionEnabled; else ocpmcp.Remove()) | ||
| | |-- rhokp.ReconcileResources() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Name the app-server explicitly.
Replace “active backend” with app-server/lightspeed-app-server; the cutover removes the ACTIVE_BACKEND pseudo-target and the controller invokes appserver.ReconcileAppServerDeployment() directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/how/reconciliation.md at line 31, Update the reconciliation
documentation entry around rhokp.ReconcileResources to name the target
explicitly as app-server or lightspeed-app-server instead of “active backend.”
Remove references to the ACTIVE_BACKEND pseudo-target and reflect that the
controller directly invokes appserver.ReconcileAppServerDeployment().
| 19. Serving-cert watchers restart the server Deployment then the app-server (`lightspeed-app-server`) and touch `lightspeed-agentic-configuration`: | ||
| - OTEL: `lightspeed-otel-collector-cert` → `RestartOtelCollector` → `RestartAppServer` → `TouchAgenticConfiguration` | ||
| - MCP: `openshift-mcp-server-tls` → MCP restart → `RestartAppServer` → `TouchAgenticConfiguration` | ||
| - RHOKP: `lightspeed-rhokp-tls` → RHOKP restart → `RestartAppServer` → `TouchAgenticConfiguration` | ||
| 20. `RestartAppServer` order: (1) refresh client CA Secrets from `openshift-service-ca.crt` (`RefreshClientCASecrets`), (2) bump annotation `ols.openshift.io/client-ca-reload` on the handoff ConfigMap (`TouchAgenticConfiguration`), (3) **re-Get** the app-server Deployment (current resourceVersion), apply any caller Spec mutations, bump `force-reload`, Update. **Fail-closed:** if step (1) fails (source CA ConfigMap missing/empty), steps (2)–(3) are skipped so pods are not rolled with stale CA material. Retry happens on a later OLSConfig reconcile or watcher event once the source CA is ready. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(agentic-sandbox-profile|tls|rhokp)\.md$|(^|/)agentic-sandbox-profile\.md$|rhokp\.md$|tls\.md$' || true
echo
echo "== context in relevant files =="
for f in .ai/spec/what/agentic-sandbox-profile.md .ai/spec/what/tls.md .ai/spec/what/rhokp.md; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
nl -ba "$f" | sed -n '40,65p'
fi
done
echo
echo "== search RestartAppServer / TouchAgenticConfiguration =="
rg -n "RestartAppServer|TouchAgenticConfiguration|client-ca-reload|also touches|RefreshClientCASecrets" .ai/spec || true
echo
echo "== code references (if any) =="
rg -n "RestartAppServer|TouchAgenticConfiguration|client-ca-reload|RefreshClientCASecrets" --glob '!/.git/**' || trueRepository: openshift/lightspeed-operator
Length of output: 399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== context in relevant files =="
for f in .ai/spec/what/agentic-sandbox-profile.md .ai/spec/what/tls.md .ai/spec/what/rhokp.md; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,120p'
fi
done
echo
echo "== search RestartAppServer / TouchAgenticConfiguration =="
rg -n "RestartAppServer|TouchAgenticConfiguration|client-ca-reload|also touches|RefreshClientCASecrets" .ai/spec || true
echo
echo "== code references for restart callbacks =="
rg -n "RestartAppServer|TouchAgenticConfiguration|RefreshClientCASecrets|client-ca-reload" --glob '!/.git/**' || trueRepository: openshift/lightspeed-operator
Length of output: 28485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate source files =="
git ls-files | rg '(^|/)(deployment|agenticintegration|cert|watcher|reconciler)\.(go|yaml|yml)$|agenticintegration|warc|cert-watch|service-ca|deployment\.go$' || true
echo
echo "== relevant source text search =="
rg -n "RestartAppServer|RestartOtelCollector|touch.*Agentic|TouchAgenticConfiguration|client-ca-reload|force-reload|RefreshClientCASecrets" --glob '!/.git/**' || true
echo
echo "== focused file sizes =="
for f in $(git ls-files | rg '(^|/)(deployment\.go|assets\.go|.*watch.*\.go|.*cert.*\.go|.*reconciler.*\.go)$'); do
printf '%s %s\n' "$(wc -l < "$f")" "$f"
done | sort -n | tail -80Repository: openshift/lightspeed-operator
Length of output: 1205
Avoid documenting a duplicate handoff-ConfigMap touch.
RestartAppServer already refreshes CA Secrets and calls TouchAgenticConfiguration, and the refresh failure path explicitly skips steps 2–3. The watcher sequences then list a separate final TouchAgenticConfiguration, while the TLS/RHOKP rotation text adds “also touches” wording, making it read as two ConfigMap touches. Document the handoff ConfigMap touch as part of RestartAppServer and use a shared rotation description instead of duplicating it.
📍 Affects 3 files
.ai/spec/what/agentic-sandbox-profile.md#L50-L54(this comment).ai/spec/what/tls.md#L37-L39.ai/spec/what/rhokp.md#L57-L60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/what/agentic-sandbox-profile.md around lines 50 - 54, Update the
shared serving-certificate rotation documentation in
.ai/spec/what/agentic-sandbox-profile.md:50-54 to describe the handoff ConfigMap
touch only as part of RestartAppServer, removing the separate final
TouchAgenticConfiguration from each watcher sequence while preserving the
RestartAppServer order and fail-closed behavior. Update the corresponding
rotation descriptions in .ai/spec/what/tls.md:37-39 and
.ai/spec/what/rhokp.md:57-60 to reference the shared RestartAppServer flow and
remove wording that implies an additional handoff-ConfigMap touch.
| }, corev1.EnvVar{ | ||
| // Make Python ssl (and httpx) use the merged ols.pem bundle that | ||
| // includes extra_ca entries (service-ca CAs for RHOKP, MCP, OTEL, etc.) | ||
| Name: "SSL_CERT_FILE", | ||
| Value: path.Join(utils.OLSAppCertsMountRoot, utils.CertBundleVolumeName, "ols.pem"), | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any existing definition/usage of the merged bundle filename
rg -n --hidden -g '!vendor' -e 'ols\.pem' -e 'SSL_CERT_FILE' -e 'CertBundle' .
# Confirm certificate_directory wiring
rg -n -C3 'CertificateDirectory' internal/controllerRepository: openshift/lightspeed-operator
Length of output: 4198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deployment.go relevant sections =="
sed -n '1,90p' internal/controller/appserver/deployment.go
echo
sed -n '230,295p' internal/controller/appserver/deployment.go
echo "== constants =="
sed -n '1,110p' internal/controller/utils/constants.go
echo "== assets relevant lines =="
sed -n '510,585p' internal/controller/appserver/assets.go
echo "== references to certificate_directory and ExtraCAs in repo =="
rg -n --hidden -g '!vendor' -e 'certificate_directory|CertificateDirectory|extra_ca|ExtraCAs|ols\.pem|SSL_CERT_FILE' .Repository: openshift/lightspeed-operator
Length of output: 21955
🌐 Web query:
lightspeed-service osde2e certificate_directory ols.pem combined CA bundle
💡 Result:
Based on the available technical documentation for Red Hat OpenShift Lightspeed (OLS), the lightspeed-service (or OpenShift Lightspeed) interacts with Large Language Model (LLM) providers using TLS certificates to secure connections [1][2]. When connecting to external providers—such as Microsoft Azure OpenAI—that require additional TLS certificates, the service is designed to add those certificates to its trust bundle to successfully establish a connection [1][2]. The term ols.pem often appears in contexts related to custom certificate management for the OpenShift Lightspeed Service, where it acts as a file containing the certificate or the combined CA (Certificate Authority) bundle required for trusted communication between the service and the LLM endpoint. While specific directory paths like certificate_directory can vary based on the deployment configuration and the specific Operator version (such as osde2e testing environments or production installs), they are generally used to mount custom CA certificates into the lightspeed-service pod so the service can trust the provider's TLS chain [1][2]. If you are encountering issues related to certificate validation or connection failures, it is typically necessary to ensure that: 1. The correct CA bundle (often in PEM format) is provided. 2. The file is mounted into the expected certificate_directory within the container environment. 3. The OpenShift Lightspeed Operator is correctly configured to use this trust bundle when communicating with the LLM provider [1][2]. For osde2e or similar automated testing contexts, these configurations are often managed via the test suite's environment variables or configuration files that define where the service looks for its operational certificates [3][4]. If you are troubleshooting, verify the mounting path in your deployment manifest and ensure the certificate file (e.g., ols.pem) contains the full chain, including intermediate and root certificates, if required by the provider [1][2][5].
Citations:
- 1: https://docs.redhat.com/fr/documentation/red_hat_openshift_lightspeed/1.0/html-single/release_notes/index
- 2: https://docs.redhat.com/fr/documentation/red_hat_openshift_lightspeed/1.0/html/release_notes/ols-release-notes
- 3: fix cleanup command polling issue osde2e#3181
- 4: https://relatedrepos.com/gh/openshift/osde2e
- 5: https://www.thesslstore.com/knowledgebase/ssl-support/ca-bundle/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== operator image / related image references =="
rg -n --hidden -g '!vendor' -e 'lightspeed-service|app-server|appserver|OLSCAConfigMap|openshift-service-ca|service-ca|related.*image|appserver' internal api test e2e . 2>/dev/null | head -n 200
echo
echo "== deployment and test references to OLSCAConfigMap / openshift-service-ca =="
rg -n -C3 'OLSCAConfigMap|openshift-service-ca|additionalCAConfigMapRef|certificate_directory|certificateDirectory' internal/appserver internal/controller test/e2e 2>/dev/nullRepository: openshift/lightspeed-operator
Length of output: 24156
🌐 Web query:
"lightspeed-service" "CertificateDirectory" "SSL_CERT_FILE"
💡 Result:
The terms "CertificateDirectory" and "SSL_CERT_FILE" relate to different technologies often encountered in similar contexts, particularly within OpenShift environments and web server configurations [1][2][3][4]. For Red Hat OpenShift Lightspeed (OLS), the term CertificateDirectory refers to a field in the OLS operator configuration [1][2]. Specifically, it defines the path to a directory containing a certificate bundle within the application server container [1]. The OLS operator also utilizes configuration paths for TLS certificates (such as tls_certificate_path and tls_key_path) within the olsconfig.yaml file to enable HTTPS for the service [5][6]. In the context of LiteSpeed Web Server, SSL_CERT_FILE is a standard configuration parameter used to specify the path to an SSL certificate file [3][4]. LiteSpeed documentation uses this parameter alongside others, such as CA Certificate File and Private Key File, to configure SSL/TLS listeners for the web server [3][4][7]. Separately, in some software projects like lightspeed-evaluation, ssl_cert_file may appear as a configuration parameter (typically setting a path to a custom CA) used to verify SSL connections to LLM providers [8]. Users should be careful to distinguish between the specific configuration requirements of the OpenShift Lightspeed service and those of the unrelated LiteSpeed Web Server [5][3][9].
Citations:
- 1: https://github.com/openshift/lightspeed-operator/blob/8f25599e32de/internal/controller/utils/types.go
- 2: https://github.com/openshift/lightspeed-operator/blob/04b1ec12/internal/controller/types.go
- 3: https://litespeedtech.com/docs/webserver/config/virtual-host-ssl
- 4: https://docs.litespeedtech.com/lsws/ssl/
- 5: https://github.com/openshift/lightspeed-service/blob/main/README.md
- 6: https://github.com/openshift/lightspeed-service/blob/main/examples/olsconfig.yaml
- 7: https://www.litespeedtech.com/docs/litespeed-web-adc/config/listener-ssl
- 8: https://github.com/lightspeed-core/lightspeed-evaluation/blob/main/config/system.yaml
- 9: https://github.com/openshift/lightspeed-operator/blob/main/README.md
🌐 Web query:
site:github.com/openshift/lightspeed-service "certificate_directory" "ols.pem"
💡 Result:
The term "certificate_directory" is not a standard configuration key in OpenShift LightSpeed (OLS) [1][2][3]. Based on the official configuration documentation for OLS, TLS is configured within the ols_config.tls_config section of the olsconfig.yaml file using explicit file paths rather than a directory reference [2][4][3]. To configure TLS in OLS, you must specify the paths to your certificate and private key files directly in your configuration [2][4][3]: ols_config: tls_config: tls_certificate_path: /full/path/to/certs/cert.pem tls_key_path: /full/path/to/certs/key.pem If you are seeing a reference to "ols.pem," it is likely a naming convention used for a specific certificate file within a local environment or deployment, potentially generated by the project's included scripts (e.g., ./scripts/generate-certs.sh) [2][4][3]. OLS does not have a hardcoded requirement for a file named "ols.pem"; rather, it requires valid paths to the certificate and key files you define in your configuration [2][4][3]. If you need to include additional Certificate Authorities (CAs) into the OLS trust store, the configuration supports an extra_ca_certs path [3].
Citations:
- 1: https://github.com/openshift/lightspeed-service/blob/main/examples/olsconfig.yaml
- 2: https://github.com/openshift/lightspeed-service/
- 3: https://github.com/openshift/lightspeed-service
- 4: https://github.com/openshift/lightspeed-service/blob/main/README.md
Confirm ols.pem is guaranteed before relying on SSL_CERT_FILE.
CertificateDirectory is /etc/certs/cert-bundle, but this repo does not define what filename lightspeed-service writes there or whether it exists before the app creates TLS contexts. Avoid assuming ols.pem; either use the real bundle filename generated by the service or point SSL_CERT_FILE at an operator-created file in this volume/config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/appserver/deployment.go` around lines 59 - 64, Update the
SSL_CERT_FILE value in the deployment environment configuration to reference the
certificate bundle filename actually generated by lightspeed-service or an
operator-managed file guaranteed to exist in the configured certificate volume.
Remove the hardcoded ols.pem assumption while preserving the existing
OLSAppCertsMountRoot and CertBundleVolumeName mount path.
78cf1ec to
187996f
Compare
openshift-ci
Bot
removed
the
needs-rebase
187996f to
281ca0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/olsconfig_controller.go (1)
317-320: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTreat the OTEL collector disablement case consistently.
internal/controller/olsconfig_controller.go:317-320and450-453comment out both resource and deployment reconciliation, but the collector controller still generates owner references and the operator status path never sets aDisabledcondition forTypeOtelCollectorReady. If a cluster previously had collector resources deployed, this leaves the status frozen instead of reflecting the current disabled state. If full disablement is temporary, add a matchingDisabledstatus entry for now.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/olsconfig_controller.go` around lines 317 - 320, Update the OTEL collector disablement handling in the OLSConfig reconciliation and status paths: when collector reconciliation is disabled, stop generating collector owner references and add a Disabled condition for TypeOtelCollectorReady. Use the existing disabled-condition pattern and ensure previously deployed resources no longer leave the collector status frozen.
🧹 Nitpick comments (7)
internal/controller/rhokp/assets.go (1)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
utils.DefaultLabels()instead of a custom label map.
selectorLabels()hand-builds the standardapp.kubernetes.io/*labels instead of delegating to the shared helper, risking label-scheme drift from other operands (this map is also reused bydeployment.go).As per path instructions, "Use
utils.DefaultLabels()for consistent labeling in asset generation functions."#!/bin/bash # Inspect DefaultLabels signature and how other components use it ast-grep run --pattern 'func DefaultLabels($$$) $_ { $$$ }' --lang go internal/controller/utils rg -n 'utils\.DefaultLabels\(' --type=go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/rhokp/assets.go` around lines 17 - 25, The selectorLabels function should delegate to utils.DefaultLabels() instead of maintaining a custom standard-label map. Replace its hand-built label values with the shared helper while preserving the labels required by deployment.go and asset generation.Source: Path instructions
internal/controller/rhokp/reconciler.go (2)
1-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a package doc comment.
reconciler.gohas no package-level doc comment describing what the RHOKP component does.As per path instructions, "Create package doc comment explaining what the component does in reconciler.go files."
📝 Proposed fix
+// Package rhokp reconciles the standalone RHOKP (Solr/OKP) Deployment, Service, +// NetworkPolicy, and TLS material used by the app server and agentic sandbox. package rhokp import (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/rhokp/reconciler.go` around lines 1 - 17, Add a package-level documentation comment in reconciler.go explaining that the rhokp package reconciles the RHOKP component and manages its Kubernetes resources. Place it immediately before the package declaration and keep the description concise and accurate.Source: Path instructions
77-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: helper names reference "ConsolePlugin" but are reused for RHOKP.
ReconcileConsolePluginService/WaitForConsolePluginTLSSecretare generic service/TLS-wait helpers being reused here for a non-console-plugin operand, which is a bit confusing to read. Consider a more neutral name if/when convenient.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/rhokp/reconciler.go` around lines 77 - 87, Rename the generic helpers ReconcileConsolePluginService and WaitForConsolePluginTLSSecret to neutral names, then update their call sites in reconcileService and reconcileTLSSecret while preserving their existing behavior.internal/controller/rhokp/deployment.go (1)
149-171: 🩺 Stability & Availability | 🔵 TrivialEmptyDir Solr data is wiped on every pod restart/reschedule.
Solr's index (up to 75Gi) lives in an
EmptyDir, so any restart (rollout, node drain, crash, OOM) loses the index and requires a full rebuild. Given the startup probe is tuned with generous initial delay/failure-threshold constants, rebuild time appears non-trivial, which could mean visible search-degradation windows after every restart. If this is an accepted trade-off (e.g., rebuildable from a durable source), it'd help to confirm that's documented; otherwise consider a PVC-backed volume for the Solr data directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/rhokp/deployment.go` around lines 149 - 171, Replace the EmptyDir-backed Solr data volume in the deployment’s Volumes configuration with a PVC-backed volume using the appropriate durable claim reference, while preserving the RHOKPSolrDataVolumeName mount identity and existing size requirement through the storage claim configuration. Ensure the Solr data directory remains available across pod restarts and rescheduling.internal/controller/watchers/watchers.go (1)
393-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestart log wording is misleading for the agentic-configuration pseudo-target.
restartDeploymentlogs"restarted deployment"/"failed to restart deployment"for every entry inaffectedDeployments, includingutils.AgenticConfigurationConfigMapName, which isn't a deployment — it's a ConfigMap touch viatouchAgenticConfigurationFunc. This can confuse debugging during CA-rotation incidents.♻️ Suggested tweak
err := restartFunc(r, ctx) if err != nil { - r.GetLogger().Error(err, "failed to restart deployment", + r.GetLogger().Error(err, "failed to apply restart action", "deployment", depName, "resource", name, "namespace", namespace) // Continue with other deployments } else { - r.GetLogger().Info("restarted deployment", + r.GetLogger().Info("applied restart action", "deployment", depName, "resource", name, "namespace", namespace) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/watchers/watchers.go` around lines 393 - 413, Update restartDeployment to use resource-appropriate log wording for the utils.AgenticConfigurationConfigMapName entry handled by touchAgenticConfigurationFunc, while retaining deployment wording for actual deployment targets. Apply the distinction to both success and failure messages without changing restart behavior.internal/controller/appserver/assets_test.go (1)
2465-2475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the generated OTEL audit contract asserted.
E2E pipeline readiness does not make
endpointandtls_modenondeterministic. Omitting both assertions allows malformed audit configuration to pass unit tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/assets_test.go` around lines 2465 - 2475, Update serviceAuditYAMLMatcher to restore the OTEL contract assertions: compute the collector endpoint using the existing service, namespace, and port symbols, and include the otel endpoint and tls_mode expectations alongside logging in the returned MatchAllKeys matcher. Remove the temporary commented-out assertions.internal/controller/appserver/deployment_test.go (1)
729-761: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert that BYOK-only mode removes RHOKP CA wiring.
This verifies only sidecar and env removal. Also assert that
rhokp-cais absent from volumes and the app container’s mounts; otherwise a regression can leave Pods dependent on a Secret for a disabled operand.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/deployment_test.go` around lines 729 - 761, Extend the “should not include RHOKP sidecar even when byokRAGOnly is true” test to assert that the generated deployment has no volume named “rhokp-ca” and that the “lightspeed-service-api” container has no corresponding volume mount. Preserve the existing sidecar and environment-variable assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/what/app-server.md:
- Line 12: Update the Deployment Composition section in app-server.md to remove
RHOKP from the app-server sidecar description. Describe only the data collector
as a possible sidecar, while preserving the standalone RHOKP deployment behavior
and other composition details.
In @.ai/spec/what/reconciliation.md:
- Line 20: Remove the completed RHOKP cutover from the Planned Changes section
in reconciliation.md, or narrow the entry to only genuinely unfinished work such
as ServiceMonitor coverage. Keep the implemented RHOKP Phase 1/Phase 2
integration and its existing conditional behavior unchanged.
- Line 31: Update the Status Conditions section in reconciliation.md to document
RHOKPReady as a Phase 2 readiness condition, including that it is NotConfigured
when byokRAGOnly is enabled and that this state does not block overall
readiness.
In `@api/v1alpha1/olsconfig_types.go`:
- Around line 410-412: Regenerate the operator bundle manifests from the updated
CRD bases so the bundle’s spec.olsConfig.deploymentConfig.rhokp schema matches
the RHOKPContainer Config definition in OLSConfig. Use the repository’s bundle
generation command, such as make bundle or hack/update_bundle.sh with the
appropriate version, and update the generated bundle manifest rather than
hand-editing the schema.
In `@internal/controller/appserver/assets.go`:
- Around line 1067-1103: Correct the error mappings in the clientCASecrets
table: change the OTEL entry’s ErrDelete to use a new
ErrDeleteAgenticOtelCASecret constant, and change the RHOKP entry’s ErrSource to
use a new ErrGetAgenticRHOKPCASourceConfigMap constant. Define both distinct
constants in errors.go with messages appropriate to deletion of the OTEL CA
Secret and reading the RHOKP shared source ConfigMap, respectively.
---
Outside diff comments:
In `@internal/controller/olsconfig_controller.go`:
- Around line 317-320: Update the OTEL collector disablement handling in the
OLSConfig reconciliation and status paths: when collector reconciliation is
disabled, stop generating collector owner references and add a Disabled
condition for TypeOtelCollectorReady. Use the existing disabled-condition
pattern and ensure previously deployed resources no longer leave the collector
status frozen.
---
Nitpick comments:
In `@internal/controller/appserver/assets_test.go`:
- Around line 2465-2475: Update serviceAuditYAMLMatcher to restore the OTEL
contract assertions: compute the collector endpoint using the existing service,
namespace, and port symbols, and include the otel endpoint and tls_mode
expectations alongside logging in the returned MatchAllKeys matcher. Remove the
temporary commented-out assertions.
In `@internal/controller/appserver/deployment_test.go`:
- Around line 729-761: Extend the “should not include RHOKP sidecar even when
byokRAGOnly is true” test to assert that the generated deployment has no volume
named “rhokp-ca” and that the “lightspeed-service-api” container has no
corresponding volume mount. Preserve the existing sidecar and
environment-variable assertions.
In `@internal/controller/rhokp/assets.go`:
- Around line 17-25: The selectorLabels function should delegate to
utils.DefaultLabels() instead of maintaining a custom standard-label map.
Replace its hand-built label values with the shared helper while preserving the
labels required by deployment.go and asset generation.
In `@internal/controller/rhokp/deployment.go`:
- Around line 149-171: Replace the EmptyDir-backed Solr data volume in the
deployment’s Volumes configuration with a PVC-backed volume using the
appropriate durable claim reference, while preserving the
RHOKPSolrDataVolumeName mount identity and existing size requirement through the
storage claim configuration. Ensure the Solr data directory remains available
across pod restarts and rescheduling.
In `@internal/controller/rhokp/reconciler.go`:
- Around line 1-17: Add a package-level documentation comment in reconciler.go
explaining that the rhokp package reconciles the RHOKP component and manages its
Kubernetes resources. Place it immediately before the package declaration and
keep the description concise and accurate.
- Around line 77-87: Rename the generic helpers ReconcileConsolePluginService
and WaitForConsolePluginTLSSecret to neutral names, then update their call sites
in reconcileService and reconcileTLSSecret while preserving their existing
behavior.
In `@internal/controller/watchers/watchers.go`:
- Around line 393-413: Update restartDeployment to use resource-appropriate log
wording for the utils.AgenticConfigurationConfigMapName entry handled by
touchAgenticConfigurationFunc, while retaining deployment wording for actual
deployment targets. Apply the distinction to both success and failure messages
without changing restart behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d2a00b88-612b-45b3-ade1-2e80c0bafbc1
⛔ Files ignored due to path filters (1)
config/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (39)
.ai/spec/how/project-structure.md.ai/spec/how/reconciliation.md.ai/spec/what/agentic-sandbox-profile.md.ai/spec/what/app-server.md.ai/spec/what/crd-api.md.ai/spec/what/ocpmcp.md.ai/spec/what/reconciliation.md.ai/spec/what/resource-lifecycle.md.ai/spec/what/rhokp.md.ai/spec/what/tls.mdAGENTS.mdapi/v1alpha1/olsconfig_types.gocmd/main.gointernal/controller/agenticintegration/assets.gointernal/controller/agenticintegration/assets_test.gointernal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/appserver/deployment.gointernal/controller/appserver/deployment_test.gointernal/controller/appserver/reconciler.gointernal/controller/appserver/reconciler_test.gointernal/controller/appserver/rhokp.gointernal/controller/appserver/rhokp_test.gointernal/controller/appserver/suite_test.gointernal/controller/ocpmcp/reconciler.gointernal/controller/ocpmcp/reconciler_test.gointernal/controller/olsconfig_controller.gointernal/controller/olsconfig_helpers.gointernal/controller/olsconfig_helpers_test.gointernal/controller/rhokp/assets.gointernal/controller/rhokp/deployment.gointernal/controller/rhokp/reconciler.gointernal/controller/rhokp/reconciler_test.gointernal/controller/rhokp/suite_test.gointernal/controller/utils/constants.gointernal/controller/utils/errors.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.go
💤 Files with no reviewable changes (3)
- internal/controller/appserver/rhokp_test.go
- internal/controller/appserver/rhokp.go
- internal/controller/ocpmcp/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (10)
- .ai/spec/what/resource-lifecycle.md
- .ai/spec/what/ocpmcp.md
- .ai/spec/what/agentic-sandbox-profile.md
- .ai/spec/how/project-structure.md
- .ai/spec/how/reconciliation.md
- .ai/spec/what/crd-api.md
- AGENTS.md
- .ai/spec/what/tls.md
- internal/controller/olsconfig_helpers_test.go
- .ai/spec/what/rhokp.md
| // RHOKP standalone deployment settings (Solr / OKP). | ||
| // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="RHOKP Container" | ||
| RHOKPContainer ContainerConfig `json:"rhokp,omitempty"` | ||
| RHOKPContainer Config `json:"rhokp,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm CRD schema reflects the Config type for the rhokp field (not ContainerConfig-shaped, e.g. no replicas subfield expected)
fd -e yaml . config/crd/bases bundle/manifests 2>/dev/null | xargs rg -n -A15 'rhokp:' 2>/dev/nullRepository: openshift/lightspeed-operator
Length of output: 3692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== API type around RHOKPContainer =="
sed -n '380,430p' api/v1alpha1/olsconfig_types.go
echo
echo "== Find ContainerConfig and Config definitions/usages =="
rg -n "type (ContainerConfig|Config) |RHOKPContainer|ContainerConfig\\$" api/v1alpha1 config/crd/bases/bundle/manifests -g '*.go' -g '*.yaml'
echo
echo "== CRD rhokp property schema around replicas/features =="
sed -n '1590,1645p' config/crd/bases/ols.openshift.io_olsconfigs.yaml
sed -n '1590,1645p' bundle/manifests/ols.openshift.io_olsconfigs.yaml
echo
echo "== Bundle vs CRD generated changes/stat for olsconfigs manifests =="
git diff --stat -- config/crd/bases/ols.openshift.io_olsconfigs.yaml bundle/manifests/ols.openshift.io_olsconfigs.yaml || trueRepository: openshift/lightspeed-operator
Length of output: 3772
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Config and ContainerConfig definitions =="
sed -n '420,505p' api/v1alpha1/olsconfig_types.go
echo
echo "== CRD and bundle files containing olsconfigs =="
git ls-files | rg '(^config/crd/bases/ols\.openshift\.io_olsconfigs\.yaml$|^bundle/manifests/ols\.openshift\.io_olsconfigs\.yaml$|config/crd/bases.*olsconfigs.*\.yaml$|bundle/manifests.*olsconfigs.*\.yaml$)' || true
echo
echo "== Bundle manifest CRD rhokp schema excerpt =="
sed -n '1585,1625p' bundle/manifests/ols.openshift.io_olsconfigs.yaml
echo
echo "== CRD base rhokp schema excerpt =="
sed -n '1585,1635p' config/crd/bases/ols.openshift.io_olsconfigs.yaml
echo
echo "== Makefile generated-manifest target context =="
sed -n '1,220p' Makefile
echo
echo "== Generate/deepcopy marker and generated usage =="
rg -n "kubebuilder:default=|kubebuilder:validation:|replica|replicas|DeepCopyInto|DeepCopy" api/v1alpha1/olsconfig_types.go api/v1alpha1/zz_generated.deepcopy.go -g '*.go'Repository: openshift/lightspeed-operator
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Bundle manifest exact rhokp properties =="
sed -n '1600,1685p' bundle/manifests/ols.openshift.io_olsconfigs.yaml
echo
echo "== CRD base exact rhokp properties =="
sed -n '1600,1695p' config/crd/bases/ols.openshift.io_olsconfigs.yaml
echo
echo "== Compare rhokp schema blocks across manifests =="
python3 - <<'PY'
from pathlib import Path
import re
paths = [
("config/crd/bases/ols.openshift.io_olsconfigs.yaml", "config/crd/bases/ols.openshift.io_olsconfigs.yaml"),
("bundle/manifests/ols.openshift.io_olsconfigs.yaml", "bundle/manifests/ols.openshift.io_olsconfigs.yaml"),
]
for path, label in paths:
text = Path(path).read_text()
m = re.search(r"\s+r[hH]o[kK]p:\s+(.*?)\n\s+tolerations:", text, re.S)
print(f"\n--- {label} ---")
print(m.group(0) if m else "rhokp block NOT found")
PYRepository: openshift/lightspeed-operator
Length of output: 16883
Regenerate the bundle manifest from the CRD bases for spec.olsConfig.deploymentConfig.rhokp.
The api/v1alpha1/olsconfig_types.go change is matched by config/crd/bases/ols.openshift.io_olsconfigs.yaml, but bundle/manifests/ols.openshift.io_olsconfigs.yaml still uses the old ContainerConfig-shaped rhokp schema. Regenerate with make bundle or hack/update_bundle.sh -v <version> so the operator bundle matches the CRD.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/olsconfig_types.go` around lines 410 - 412, Regenerate the
operator bundle manifests from the updated CRD bases so the bundle’s
spec.olsConfig.deploymentConfig.rhokp schema matches the RHOKPContainer Config
definition in OLSConfig. Use the repository’s bundle generation command, such as
make bundle or hack/update_bundle.sh with the appropriate version, and update
the generated bundle manifest rather than hand-editing the schema.
Source: Path instructions
281ca0c to
59f0729
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
api/v1alpha1/olsconfig_types.go (1)
410-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
Config.Replicasdoc comment to mention RHOKP.
RHOKPContainernow uses the sharedConfigtype (line 412), butConfig.Replicas's doc comment (lines 446-448, unchanged) explicitly enumerates which components support configurable replicas vs. are pinned to 1 (Postgres, Console, Agentic Console, Alerts Adapter, OTEL Collector, Agentic Sandbox) — RHOKP isn't listed on either side, even thoughrhokp/deployment.gocallsApplyPodDeploymentConfig(..., false), which pins replicas to 1. This leaves CRD consumers uncertain whether settingrhokp.replicashas any effect.📝 Proposed doc fix
// Defines the number of desired pods. Default: "1" // Note: Replicas are configurable for APIContainer and MCP server (mcpServer). - // For PostgreSQL, Console, Agentic Console, Alerts Adapter, OTEL Collector, and + // For PostgreSQL, RHOKP, Console, Agentic Console, Alerts Adapter, OTEL Collector, and // Agentic Sandbox (spec.agenticOLS.agenticSandboxConfig), the number of replicas is always set to 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/olsconfig_types.go` around lines 410 - 412, Update the Config.Replicas documentation to explicitly list RHOKP among components whose replica count is pinned to 1. Anchor the change to the Config type’s Replicas doc comment and reflect the behavior of rhokp/deployment.go calling ApplyPodDeploymentConfig with false; leave the existing replica documentation for other components unchanged.internal/controller/appserver/deployment.go (1)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrphaned/misplaced comment referencing a test-only helper.
This comment documents
deploymentContainerCount, but that function lives only insuite_test.go(a test helper) and isn't defined or used in this file. It sits immediately beforeGenerateOLSDeployment, which it doesn't describe. Likely a leftover from refactoring the RHOKP sidecar removal — consider removing or relocating it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/deployment.go` at line 76, Remove the orphaned deploymentContainerCount comment immediately before GenerateOLSDeployment, since it documents a test-only helper that is not defined or used in this file.internal/controller/appserver/assets.go (1)
1105-1118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReused OTEL-named constant as the generic source-CA key for all three components.
generateClientCAreadscaCM.Data[utils.AppOtelCollectorCACertFile]for OTEL, MCP, and RHOKP alike — functionally correct (same shared ConfigMap/key), but naming the key constant after OTEL specifically is confusing when it's now used generically. Consider introducing a neutrally-named constant (e.g.ServiceCABundleKey) for this shared-source lookup, keepingAppOtelCollectorCACertFileonly for the OTEL-specific mount file name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/assets.go` around lines 1105 - 1118, Introduce a neutrally named constant for the shared source CA key and update generateClientCA to use it when reading and validating the ConfigMap entry. Reserve AppOtelCollectorCACertFile for the OTEL-specific mounted filename, and update any related references to the shared key without changing behavior.internal/controller/rhokp/assets.go (1)
58-97: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNetwork policy allows ingress from any pod in the namespace, not just app-server.
PodSelector: &metav1.LabelSelector{}with noNamespaceSelectormatches every pod in the operator's namespace (postgres, otel-collector, console-ui, alerts-adapter, etc.), not just the intended app-server/sandbox consumers mentioned in the comment. Consider scoping the peer selector to the specific label(s) of known consumers to reduce lateral-movement surface within the namespace.🔒 Suggested tightening
Ingress: []networkingv1.NetworkPolicyIngressRule{ { From: []networkingv1.NetworkPolicyPeer{ { - PodSelector: &metav1.LabelSelector{}, + PodSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app.kubernetes.io/name", + Operator: metav1.LabelSelectorOpIn, + Values: []string{utils.OLSAppServerDeploymentName /* add future sandbox consumer names here */}, + }, + }, + }, }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/rhokp/assets.go` around lines 58 - 97, Update GenerateNetworkPolicy’s ingress peer selector to match only the labels used by the intended app-server and sandbox consumer pods, rather than the empty PodSelector that permits every pod in the namespace. Preserve HTTPS port 8443 and the existing policy structure, and reuse the established consumer label symbols where available.internal/controller/olsconfig_helpers_test.go (1)
821-856: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the new
RHOKPTLSWatchEnabledgating.This file thoroughly tests
OpenShiftMCPServerTLSWatchEnabledenable/disable behavior viaannotateExternalResources, but there's no equivalent test forRHOKPTLSWatchEnabled, whichcmd/main.gostates is gated by!byokRAGOnlyfor the new RHOKP TLS secret watcher entry. Since this is new reconciliation functionality, add parallel tests (enable whenByokRAGOnlyis false/unset, disable when true) mirroring the existing MCP tests.As per coding guidelines, "Write unit tests in co-located
*_test.gofiles for new reconciliation functionality."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/olsconfig_helpers_test.go` around lines 821 - 856, Extend the watcher tests around annotateExternalResources with parallel RHOKP TLS coverage: verify RHOKPTLSWatchEnabled becomes true when ByokRAGOnly is unset or false, and becomes false when ByokRAGOnly is true. Mirror the existing OpenShiftMCPServerTLSWatchEnabled tests, including shouldWatchSecret checks for the RHOKP TLS secret and confirming its SystemResources entry remains present when gating is disabled.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/what/app-server.md:
- Around line 122-123: Update the Planned Changes section by removing the
already-implemented classic-to-agentic sandbox handoff and CA Secret ownership
text. Retain only the explicitly deferred optional agentic auto-injection item,
preserving its OLS-3594 reference.
In `@internal/controller/olsconfig_controller.go`:
- Around line 346-348: Update the disabled reconcile cleanup around
wasComponentEnabled and rhokp.Remove so RHOKP resources are removed whenever
ByokRAGOnly is enabled, without depending on prior RHOKPReady status. Use the
idempotent deletion behavior that tolerates missing resources, while preserving
the existing resourceFailures handling for actual removal errors.
In `@internal/controller/rhokp/reconciler.go`:
- Around line 1-2: Add a package-level documentation comment immediately before
the package declaration in reconciler.go, describing the purpose and
responsibilities of the RHOKP component managed by package rhokp. Keep the
comment concise and aligned with the package’s actual reconciler behavior.
---
Nitpick comments:
In `@api/v1alpha1/olsconfig_types.go`:
- Around line 410-412: Update the Config.Replicas documentation to explicitly
list RHOKP among components whose replica count is pinned to 1. Anchor the
change to the Config type’s Replicas doc comment and reflect the behavior of
rhokp/deployment.go calling ApplyPodDeploymentConfig with false; leave the
existing replica documentation for other components unchanged.
In `@internal/controller/appserver/assets.go`:
- Around line 1105-1118: Introduce a neutrally named constant for the shared
source CA key and update generateClientCA to use it when reading and validating
the ConfigMap entry. Reserve AppOtelCollectorCACertFile for the OTEL-specific
mounted filename, and update any related references to the shared key without
changing behavior.
In `@internal/controller/appserver/deployment.go`:
- Line 76: Remove the orphaned deploymentContainerCount comment immediately
before GenerateOLSDeployment, since it documents a test-only helper that is not
defined or used in this file.
In `@internal/controller/olsconfig_helpers_test.go`:
- Around line 821-856: Extend the watcher tests around annotateExternalResources
with parallel RHOKP TLS coverage: verify RHOKPTLSWatchEnabled becomes true when
ByokRAGOnly is unset or false, and becomes false when ByokRAGOnly is true.
Mirror the existing OpenShiftMCPServerTLSWatchEnabled tests, including
shouldWatchSecret checks for the RHOKP TLS secret and confirming its
SystemResources entry remains present when gating is disabled.
In `@internal/controller/rhokp/assets.go`:
- Around line 58-97: Update GenerateNetworkPolicy’s ingress peer selector to
match only the labels used by the intended app-server and sandbox consumer pods,
rather than the empty PodSelector that permits every pod in the namespace.
Preserve HTTPS port 8443 and the existing policy structure, and reuse the
established consumer label symbols where available.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9134d323-d680-496b-a2de-abadd58ad1be
⛔ Files ignored due to path filters (1)
config/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (39)
.ai/spec/how/project-structure.md.ai/spec/how/reconciliation.md.ai/spec/what/agentic-sandbox-profile.md.ai/spec/what/app-server.md.ai/spec/what/crd-api.md.ai/spec/what/ocpmcp.md.ai/spec/what/reconciliation.md.ai/spec/what/resource-lifecycle.md.ai/spec/what/rhokp.md.ai/spec/what/tls.mdAGENTS.mdapi/v1alpha1/olsconfig_types.gocmd/main.gointernal/controller/agenticintegration/assets.gointernal/controller/agenticintegration/assets_test.gointernal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/appserver/deployment.gointernal/controller/appserver/deployment_test.gointernal/controller/appserver/reconciler.gointernal/controller/appserver/reconciler_test.gointernal/controller/appserver/rhokp.gointernal/controller/appserver/rhokp_test.gointernal/controller/appserver/suite_test.gointernal/controller/ocpmcp/reconciler.gointernal/controller/ocpmcp/reconciler_test.gointernal/controller/olsconfig_controller.gointernal/controller/olsconfig_helpers.gointernal/controller/olsconfig_helpers_test.gointernal/controller/rhokp/assets.gointernal/controller/rhokp/deployment.gointernal/controller/rhokp/reconciler.gointernal/controller/rhokp/reconciler_test.gointernal/controller/rhokp/suite_test.gointernal/controller/utils/constants.gointernal/controller/utils/errors.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.go
💤 Files with no reviewable changes (3)
- internal/controller/appserver/rhokp.go
- internal/controller/appserver/rhokp_test.go
- internal/controller/ocpmcp/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (8)
- .ai/spec/what/resource-lifecycle.md
- .ai/spec/how/reconciliation.md
- .ai/spec/what/ocpmcp.md
- .ai/spec/how/project-structure.md
- .ai/spec/what/tls.md
- .ai/spec/what/crd-api.md
- .ai/spec/what/rhokp.md
- AGENTS.md
| - Classic→agentic sandbox handoff: appserver owns client CA Secrets (`lightspeed-agentic-otel-ca` / `lightspeed-agentic-mcp-ca` / `lightspeed-agentic-rhokp-ca`) and mounts them; `agenticintegration` owns the handoff ConfigMap — see `agentic-sandbox-profile.md` (OLS-3683 / OLS-3684). Optional agentic auto-injection remains deferred ([OLS-3594](https://redhat.atlassian.net/browse/OLS-3594)). | ||
| - [PLANNED: OLS-3572] Agentic sandbox configuration handoff — classic operator builds base PodSpec and writes `lightspeed-sandbox-config` ConfigMap for the agentic operator. See Rules 34–36. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove implemented behavior from Planned Changes.
The current rules already document the classic-to-agentic handoff and CA Secret ownership as implemented behavior, while this bullet appears under Planned Changes without a planned marker. Remove it or narrow it to genuinely unfinished work; retain only the explicitly deferred auto-injection item.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/what/app-server.md around lines 122 - 123, Update the Planned
Changes section by removing the already-implemented classic-to-agentic sandbox
handoff and CA Secret ownership text. Retain only the explicitly deferred
optional agentic auto-injection item, preserving its OLS-3594 reference.
| } else if wasComponentEnabled(olsconfig, utils.TypeRHOKPReady) { | ||
| if err := rhokp.Remove(r, ctx); err != nil { | ||
| resourceFailures["RHOKP cleanup"] = fmt.Errorf("%s: %w", utils.ErrRemoveRHOKPResources, err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove RHOKP resources whenever ByokRAGOnly is enabled.
Cleanup currently depends on the previous RHOKPReady status. If resources exist but that condition is missing or already says Disabled—for example after a failed status update—this branch skips deletion and leaves RHOKP running despite the disabled configuration.
Call rhokp.Remove on every disabled reconcile, using an idempotent delete path that tolerates missing resources.
Proposed fix
- } else if wasComponentEnabled(olsconfig, utils.TypeRHOKPReady) {
+ } else {
if err := rhokp.Remove(r, ctx); err != nil {📝 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.
| } else if wasComponentEnabled(olsconfig, utils.TypeRHOKPReady) { | |
| if err := rhokp.Remove(r, ctx); err != nil { | |
| resourceFailures["RHOKP cleanup"] = fmt.Errorf("%s: %w", utils.ErrRemoveRHOKPResources, err) | |
| } else { | |
| if err := rhokp.Remove(r, ctx); err != nil { | |
| resourceFailures["RHOKP cleanup"] = fmt.Errorf("%s: %w", utils.ErrRemoveRHOKPResources, err) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/olsconfig_controller.go` around lines 346 - 348, Update
the disabled reconcile cleanup around wasComponentEnabled and rhokp.Remove so
RHOKP resources are removed whenever ByokRAGOnly is enabled, without depending
on prior RHOKPReady status. Use the idempotent deletion behavior that tolerates
missing resources, while preserving the existing resourceFailures handling for
actual removal errors.
| package rhokp | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a package doc comment to reconciler.go.
No doc comment precedes package rhokp explaining what the RHOKP component does. As per path instructions, "Create package doc comment explaining what the component does in reconciler.go files."
📝 Proposed doc comment
+// Package rhokp reconciles the standalone RHOKP (Red Hat Offline Knowledge Portal)
+// operand: a Deployment, Service, NetworkPolicy, and service-ca TLS Secret providing
+// an HTTPS Solr endpoint for hybrid RAG retrieval.
package rhokp📝 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.
| package rhokp | |
| // Package rhokp reconciles the standalone RHOKP (Red Hat Offline Knowledge Portal) | |
| // operand: a Deployment, Service, NetworkPolicy, and service-ca TLS Secret providing | |
| // an HTTPS Solr endpoint for hybrid RAG retrieval. | |
| package rhokp |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/rhokp/reconciler.go` around lines 1 - 2, Add a
package-level documentation comment immediately before the package declaration
in reconciler.go, describing the purpose and responsibilities of the RHOKP
component managed by package rhokp. Keep the comment concise and aligned with
the package’s actual reconciler behavior.
Source: Path instructions
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
AI Code Review — OLS-3726 (Round 1)
Score: 84/100 | Mode: jira | Verdict: needs_review
Adherence
13/13 acceptance criteria PASS. Well-structured PR that cleanly extracts RHOKP from an app-server sidecar into a standalone operand following the established ocpmcp pattern.
Code Quality Issues
1. 🔴 Missing rhokp.Remove() in finalizeOLSConfig() (must-fix)
internal/controller/olsconfig_controller.go ~line 797
finalizeOLSConfig() calls ocpmcp.Remove() to clean up the MCP TLS Secret (created by Service CA Operator, not owned by OLSConfig), but there is no parallel rhokp.Remove() call. The lightspeed-rhokp-tls Secret is also created by Service CA and not owned by OLSConfig — it will be orphaned on CR deletion. The owned resources (Deployment, Service, NetworkPolicy) are handled by deleteOwnedResources(), but the TLS Secret will persist.
Suggested fix — add after the ocpmcp.Remove() block:
r.Logger.V(1).Info("Removing RHOKP operand during finalization")
if err := rhokp.Remove(r, ctx); err != nil {
r.Logger.Error(err, "Failed to remove RHOKP during finalization")
r.Logger.V(1).Info("Proceeding with finalization despite RHOKP removal error")
}2. 🟡 Stale comment referencing removed RHOKP sidecar (should-fix)
internal/controller/appserver/deployment.go:555-557
Comment says:
// Add additional containers in a consistent order:
// 1. Data collector container (if enabled)
// 2. RHOKP Solr sidecar (if Solr hybrid RAG is configured)
But the sidecar append block was removed — only the data collector container remains. Line 557 (item 2) should be deleted.
3. 🟡 Orphan doc comment with no function body (should-fix)
internal/controller/appserver/deployment.go:76
// deploymentContainerCount returns the expected number of containers in the app-server pod.This is a free-standing doc comment with no function following it. The function only exists in suite_test.go. This line replaced the deleted rhokpHTTPProbeHandler() function and should be removed.
AI review by Claude — round 1. Fix the must-fix and re-request review.
59f0729 to
690b8ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/controller/rhokp/deployment.go (1)
229-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a blank parameter name, and wrap the
Getfailure with a get-specific error constant.Two small cleanups:
- The variadic
deploymentparameter is never used. Name it_and remove the_ = deploymentstatement.- Line 240 wraps a
Getfailure withutils.ErrUpdateRHOKPDeployment. That misreports the failed operation. Useutils.ErrGetRHOKPDeploymentif it exists, or add it tointernal/controller/utils/errors.go.♻️ Proposed change
-func Restart(r reconciler.Reconciler, ctx context.Context, deployment ...*appsv1.Deployment) error { - _ = deployment - +func Restart(r reconciler.Reconciler, ctx context.Context, _ ...*appsv1.Deployment) error { dep := &appsv1.Deployment{} err := r.Get(ctx, client.ObjectKey{Name: utils.RHOKPDeploymentName, Namespace: r.GetNamespace()}, dep) if err != nil { if errors.IsNotFound(err) { r.GetLogger().Info("RHOKP deployment not found, skipping restart", "deployment", utils.RHOKPDeploymentName) return nil } - return fmt.Errorf("%s: %w", utils.ErrUpdateRHOKPDeployment, err) + return fmt.Errorf("%s: %w", utils.ErrGetRHOKPDeployment, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/rhokp/deployment.go` around lines 229 - 241, Update Restart to use a blank variadic parameter name and remove the redundant _ = deployment assignment. Change the non-NotFound r.Get error wrapping to use utils.ErrGetRHOKPDeployment; add that constant in errors.go if it does not already exist, while preserving the existing NotFound handling.Source: Coding guidelines
internal/controller/appserver/assets.go (1)
1114-1118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a component-neutral constant name for the service-ca key.
generateClientCAis now shared by OTEL, MCP, and RHOKP. It reads the keyutils.AppOtelCollectorCACertFilefrom the sharedutils.OLSCAConfigMap. The value is correct, but the OTEL-specific constant name suggests an OTEL-only source. Add a neutral alias such asutils.ServiceCACertKeyand use it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/appserver/assets.go` around lines 1114 - 1118, Add a component-neutral alias named utils.ServiceCACertKey for the shared service CA key, then update generateClientCA to use it when reading caCM.Data and constructing the missing-key error. Preserve the existing key value and OLSCAConfigMap reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/what/app-server.md:
- Around line 96-101: Update the RHOKP resource conventions in “Resource
Conventions” to describe the EmptyDir sizeLimit as a default of approximately 75
GiB rather than a fixed value. State that the RHOKP CR’s configured
ephemeral-storage request can override this default, while preserving that the
convention applies only to CPU and memory resource defaults.
---
Nitpick comments:
In `@internal/controller/appserver/assets.go`:
- Around line 1114-1118: Add a component-neutral alias named
utils.ServiceCACertKey for the shared service CA key, then update
generateClientCA to use it when reading caCM.Data and constructing the
missing-key error. Preserve the existing key value and OLSCAConfigMap reference.
In `@internal/controller/rhokp/deployment.go`:
- Around line 229-241: Update Restart to use a blank variadic parameter name and
remove the redundant _ = deployment assignment. Change the non-NotFound r.Get
error wrapping to use utils.ErrGetRHOKPDeployment; add that constant in
errors.go if it does not already exist, while preserving the existing NotFound
handling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bdeac10a-60df-4348-8388-fd4bebd96768
⛔ Files ignored due to path filters (1)
config/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (39)
.ai/spec/how/project-structure.md.ai/spec/how/reconciliation.md.ai/spec/what/agentic-sandbox-profile.md.ai/spec/what/app-server.md.ai/spec/what/crd-api.md.ai/spec/what/ocpmcp.md.ai/spec/what/reconciliation.md.ai/spec/what/resource-lifecycle.md.ai/spec/what/rhokp.md.ai/spec/what/tls.mdAGENTS.mdapi/v1alpha1/olsconfig_types.gocmd/main.gointernal/controller/agenticintegration/assets.gointernal/controller/agenticintegration/assets_test.gointernal/controller/appserver/assets.gointernal/controller/appserver/assets_test.gointernal/controller/appserver/deployment.gointernal/controller/appserver/deployment_test.gointernal/controller/appserver/reconciler.gointernal/controller/appserver/reconciler_test.gointernal/controller/appserver/rhokp.gointernal/controller/appserver/rhokp_test.gointernal/controller/appserver/suite_test.gointernal/controller/ocpmcp/reconciler.gointernal/controller/ocpmcp/reconciler_test.gointernal/controller/olsconfig_controller.gointernal/controller/olsconfig_helpers.gointernal/controller/olsconfig_helpers_test.gointernal/controller/rhokp/assets.gointernal/controller/rhokp/deployment.gointernal/controller/rhokp/reconciler.gointernal/controller/rhokp/reconciler_test.gointernal/controller/rhokp/suite_test.gointernal/controller/utils/constants.gointernal/controller/utils/errors.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.go
💤 Files with no reviewable changes (3)
- internal/controller/appserver/rhokp_test.go
- internal/controller/appserver/rhokp.go
- internal/controller/ocpmcp/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (29)
- .ai/spec/what/resource-lifecycle.md
- api/v1alpha1/olsconfig_types.go
- internal/controller/appserver/reconciler_test.go
- internal/controller/ocpmcp/reconciler_test.go
- internal/controller/olsconfig_helpers.go
- internal/controller/appserver/suite_test.go
- internal/controller/appserver/reconciler.go
- internal/controller/agenticintegration/assets_test.go
- internal/controller/olsconfig_controller.go
- internal/controller/rhokp/assets.go
- .ai/spec/what/agentic-sandbox-profile.md
- cmd/main.go
- internal/controller/utils/types.go
- internal/controller/watchers/watchers.go
- .ai/spec/how/reconciliation.md
- .ai/spec/what/ocpmcp.md
- internal/controller/agenticintegration/assets.go
- .ai/spec/what/tls.md
- .ai/spec/how/project-structure.md
- internal/controller/rhokp/reconciler_test.go
- internal/controller/olsconfig_helpers_test.go
- .ai/spec/what/crd-api.md
- .ai/spec/what/rhokp.md
- internal/controller/rhokp/reconciler.go
- AGENTS.md
- internal/controller/appserver/assets_test.go
- internal/controller/watchers/watchers_test.go
- internal/controller/rhokp/suite_test.go
- internal/controller/appserver/deployment_test.go
| 5. RHOKP runs as a standalone Deployment (`lightspeed-rhokp`) with its own 75 GiB EmptyDir. The app-server pod no longer requires ephemeral storage for OKP. See `rhokp.md`. | ||
|
|
||
| ### Resource Conventions [OLS-3397] | ||
| 30. All operator-managed container defaults follow the [OpenShift resource conventions](https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md#resources-and-limits): defaults declare CPU and memory requests only, and do not set resource limits. This applies to the primary API container, sidecars (data collector), the standalone MCP Deployment, and the standalone RHOKP Deployment. | ||
| 31. Users may still set limits via the CRD (`spec.ols.deployment.<component>.resources`, including `spec.ols.deployment.rhokp.resources`) if their environment requires it. The CRD uses standard `corev1.ResourceRequirements` which accepts both requests and limits. | ||
| 32. [PLANNED: OLS-3697] The RHOKP standalone Deployment's ~75 GiB EmptyDir sizeLimit is unchanged by this convention — it applies only to CPU and memory. | ||
| 32. The RHOKP standalone Deployment's ~75 GiB EmptyDir sizeLimit is unchanged by this convention — it applies only to CPU and memory. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document 75 GiB as the default, not a fixed limit.
internal/controller/rhokp/deployment.go replaces the default 75Gi size with the RHOKP ephemeral-storage request when one is configured. Update these rules so operators know that the CR can override the EmptyDir sizeLimit.
Proposed wording
-5. RHOKP runs as a standalone Deployment (`lightspeed-rhokp`) with its own 75 GiB EmptyDir.
+5. RHOKP runs as a standalone Deployment (`lightspeed-rhokp`) with a default 75 GiB EmptyDir. The RHOKP `ephemeral-storage` request overrides this sizeLimit when configured.📝 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.
| 5. RHOKP runs as a standalone Deployment (`lightspeed-rhokp`) with its own 75 GiB EmptyDir. The app-server pod no longer requires ephemeral storage for OKP. See `rhokp.md`. | |
| ### Resource Conventions [OLS-3397] | |
| 30. All operator-managed container defaults follow the [OpenShift resource conventions](https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md#resources-and-limits): defaults declare CPU and memory requests only, and do not set resource limits. This applies to the primary API container, sidecars (data collector), the standalone MCP Deployment, and the standalone RHOKP Deployment. | |
| 31. Users may still set limits via the CRD (`spec.ols.deployment.<component>.resources`, including `spec.ols.deployment.rhokp.resources`) if their environment requires it. The CRD uses standard `corev1.ResourceRequirements` which accepts both requests and limits. | |
| 32. [PLANNED: OLS-3697] The RHOKP standalone Deployment's ~75 GiB EmptyDir sizeLimit is unchanged by this convention — it applies only to CPU and memory. | |
| 32. The RHOKP standalone Deployment's ~75 GiB EmptyDir sizeLimit is unchanged by this convention — it applies only to CPU and memory. | |
| 5. RHOKP runs as a standalone Deployment (`lightspeed-rhokp`) with a default 75 GiB EmptyDir. The RHOKP `ephemeral-storage` request overrides this sizeLimit when configured. The app-server pod no longer requires ephemeral storage for OKP. See `rhokp.md`. | |
| ### Resource Conventions [OLS-3397] | |
| 30. All operator-managed container defaults follow the [OpenShift resource conventions](https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md#resources-and-limits): defaults declare CPU and memory requests only, and do not set resource limits. This applies to the primary API container, sidecars (data collector), the standalone MCP Deployment, and the standalone RHOKP Deployment. | |
| 31. Users may still set limits via the CRD (`spec.ols.deployment.<component>.resources`, including `spec.ols.deployment.rhokp.resources`) if their environment requires it. The CRD uses standard `corev1.ResourceRequirements` which accepts both requests and limits. | |
| 32. The RHOKP standalone Deployment's ~75 GiB EmptyDir sizeLimit is unchanged by this convention — it applies only to CPU and memory. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/what/app-server.md around lines 96 - 101, Update the RHOKP resource
conventions in “Resource Conventions” to describe the EmptyDir sizeLimit as a
default of approximately 75 GiB rather than a fixed value. State that the RHOKP
CR’s configured ephemeral-storage request can override this default, while
preserving that the convention applies only to CPU and memory resource defaults.
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
AI Code Review — OLS-3726 (Round 2)
Score: 100/100 | Mode: jira | Verdict: pass
Adherence
13/13 acceptance criteria PASS.
Round 1 Issues — All Resolved
- ✅ Missing
rhokp.Remove()in finalizer — added atolsconfig_controller.go:802 - ✅ Stale sidecar comment — updated to note standalone pattern
- ✅ Orphan doc comment — removed
No new issues found. PR is ready for human review.
AI review by Claude — round 2 (final). Remaining review is for the human reviewer.
|
/lgtm |
|
/test ci/prow/bundle-e2e-4-21 |
|
/test bundle-e2e-4-21 |
|
@blublinsky: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
blublinsky
changed the title
|
@blublinsky: This pull request references OLS-3726 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blublinsky 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-ci
Bot
added
the
approved
3 participants
Description
Summary
Replaces the RHOKP (Red Hat Offline Knowledge Portal) sidecar container in the
app-server pod with a standalone Deployment, Service, and NetworkPolicy — following
the same pattern as the OpenShift MCP server operand.
Changes
New operand:
internal/controller/rhokp/lightspeed-rhokp) with service-ca TLS annotation on port 8443!ByokRAGOnlyin the main controller;Remove()cleans up when disabledApp-server changes
SolrHTTPBasenow points to standalone service (https://lightspeed-rhokp.<ns>.svc:8443)lightspeed-agentic-rhokp-caclient CA Secret (table-driven alongside OTEL/MCP)ExtraCAsSSL_CERT_FILEenv var pointing to the mergedols.pembundle sohttpxtrusts service-ca-signed certificates for all outbound HTTPS (RHOKP, MCP, OTEL)
Watcher & lifecycle
lightspeed-rhokp-tlsto system resource watchers (triggers RHOKP restart,app-server restart, and agentic ConfigMap touch on TLS rotation)
"ACTIVE_BACKEND"references withutils.OLSAppServerDeploymentNameSimplifications
handle CA rotation detection for all operands consistently
(
RefreshClientCASecrets) for OTEL, MCP, and RHOKPStatus condition
RHOKPReadycondition (True when deployed, NotConfigured when ByokRAGOnly)Testing
Unit tests for all new
rhokp/package filesUpdated app-server tests (removed sidecar assertions, added RHOKP CA expectations)
Verified end-to-end: operator deploys all resources, app-server connects to
standalone RHOKP over HTTPS with service-ca trust
Refactor
[ x] New feature
Bug fix
CVE fix
Optimization
Documentation Update
Configuration Update
Bump-up dependent library
Related Tickets & Documents
https://redhat.atlassian.net/browse/OLS-3726
https://redhat.atlassian.net/browse/OLS-3726
Checklist before requesting a review
Testing
Summary by CodeRabbit
byokRAGOnly-based enable/disable behavior.