OLS-3799 Add wait-for-rhokp init container to app-server deployment - #1921
OLS-3799 Add wait-for-rhokp init container to app-server deployment#1921vimalk78 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds a RHOKP/Solr readiness init container. Non-BYOK-only deployments include it. BYOK-only deployments omit it. The container polls HTTPS with a mounted CA certificate, bounded backoff, restricted security settings, and a 360-second timeout. ChangesRHOKP readiness integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GenerateOLSDeployment
participant GenerateRHOKPWaitInitContainer
participant RHOKPSolr
GenerateOLSDeployment->>GenerateRHOKPWaitInitContainer: Generate wait-for-rhokp when ByokRAGOnly is false
GenerateRHOKPWaitInitContainer-->>GenerateOLSDeployment: Return configured init container
GenerateRHOKPWaitInitContainer->>RHOKPSolr: Poll HTTPS admin/ping endpoint with CA certificate
RHOKPSolr-->>GenerateRHOKPWaitInitContainer: Readiness response or timeout
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Must-fix: cr.Namespace is always empty for the cluster-scoped OLSConfig CRD
At deployment.go:447:
initContainers = append(initContainers, utils.GenerateRHOKPWaitInitContainer(r.GetAppServerImage(), cr.Namespace))OLSConfig is scope: Cluster, so cr.Namespace is always "". This produces a URL like https://lightspeed-rhokp..svc:8443/... (double dots — invalid DNS).
Suggested fix: Drop the namespace parameter entirely and use the short service name, matching the Postgres wait pattern. Since the init container runs in the same namespace as the RHOKP service, Kubernetes DNS resolves the short name automatically:
rhokpURL := fmt.Sprintf("https://%s:%d/solr/portal-rag/select?q=*:*&rows=0&wt=json",
RHOKPServiceName, RHOOKPImageHTTPSPort)This also simplifies GenerateRHOKPWaitInitContainer — it no longer needs the namespace argument, just like GeneratePostgresWaitInitContainer only takes the image.
Note: --cacert TLS verification with a short hostname may require the service-ca cert to include the short name as a SAN. If FQDN is needed for TLS cert matching, use the constant utils.OLSNamespaceDefault instead of cr.Namespace.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Questioning the approach: should this fix live in the operator at all?
The app-server (lightspeed-service-api) already has retry logic for Solr connectivity. The root cause described here is that SolrHybridSearch uses @cached_property which permanently caches None on failure — that's a bug in the service code, not a deployment ordering problem.
The proper fix is in lightspeed-service-api: don't permanently cache a transient failure. For example, use a lazy property that retries on None, or replace @cached_property with a retry-aware initializer. This makes the service resilient regardless of startup ordering, container restarts, or network blips — situations an init container cannot help with anyway.
Adding an init container with shell scripts, CA mounts, and backoff logic in the operator is treating the symptom at the wrong layer. Compare with the Postgres wait: that exists because the app literally cannot start without a DB connection. Solr/RHOKP is not a hard prerequisite — the app should degrade gracefully and recover when it becomes available.
I'd suggest fixing the caching bug in the service and closing this PR. Happy to discuss if there's additional context I'm missing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/controller/utils/rhokp_wait_test.go`:
- Around line 19-32: Strengthen the test for GenerateRHOKPWaitInitContainer to
validate the complete readiness command and its behavior, not just command
length or marker text. Control curl, date, and sleep so the test covers the
successful probe, timeout exit status, bounded backoff, and the 360-second
RHOKPWaitMaxSeconds boundary; ensure the generated command does not perform a
probe or sleep after the deadline. Assert the full command contents, including
the complete Solr query and expected exit statuses, while preserving the
existing container-spec checks.
🪄 Autofix
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: Enterprise
Run ID: 308fa67e-bdc1-408a-9c49-fc544800ad31
📒 Files selected for processing (6)
internal/controller/appserver/assets_test.gointernal/controller/appserver/deployment.gointernal/controller/appserver/deployment_test.gointernal/controller/utils/constants.gointernal/controller/utils/rhokp_wait.gointernal/controller/utils/rhokp_wait_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/controller/utils/constants.go
- internal/controller/appserver/assets_test.go
- internal/controller/appserver/deployment_test.go
- internal/controller/appserver/deployment.go
- internal/controller/utils/rhokp_wait.go
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
Adversarial Review
Overall the approach is sound — gating on !ByokRAGOnly is consistent, the timeout matches the RHOKP startup probe budget, and the exponential backoff is well-implemented. Four issues found in inline comments below.
| [ "$sleep_sec" -gt "$max_sleep" ] && sleep_sec="$max_sleep" | ||
| } | ||
|
|
||
| while true; do |
There was a problem hiding this comment.
Missing command -v curl guard
postgres_wait.go validates its tool exists before entering the poll loop:
if ! command -v pg_isready >/dev/null 2>&1; then
echo "wait-for-postgres: pg_isready not found in image" >&2
exit 1
fiThis script has no equivalent check for curl. If curl is ever removed from the app-server image, the init container will loop for 360 seconds (the curl invocation suppresses all output via >/dev/null 2>&1) before timing out — a confusing silent failure instead of a fast one.
Suggestion: add command -v curl check before the while true loop, matching the postgres pattern.
There was a problem hiding this comment.
Added command -v curl guard before the poll loop, matching the postgres pattern. Fails fast with a clear message instead of silently looping for 360s.
| }, | ||
| SecurityContext: &corev1.SecurityContext{ | ||
| AllowPrivilegeEscalation: &[]bool{false}[0], | ||
| ReadOnlyRootFilesystem: &[]bool{true}[0], |
There was a problem hiding this comment.
Incomplete SecurityContext — missing PSA "restricted" fields
This only sets AllowPrivilegeEscalation and ReadOnlyRootFilesystem. The codebase has RestrictedContainerSecurityContext() (utils/utils.go:73) that also sets:
RunAsNonRoot: trueSeccompProfile: RuntimeDefaultCapabilities: Drop ALL
These are required by the Pod Security "restricted" profile. Without them, the pod could fail admission on namespaces with PSA enforcement.
Note: wait-for-postgres has the same gap (pre-existing), but this PR should not compound it.
Suggestion: use RestrictedContainerSecurityContext() instead of the inline SecurityContext.
There was a problem hiding this comment.
Switched to RestrictedContainerSecurityContext() — picks up RunAsNonRoot, SeccompProfile RuntimeDefault, and Drop ALL capabilities.
| // lightweight Solr select query. The RHOKP CA certificate volume must already | ||
| // be defined on the pod (AppRHOKPCACertVolumeName). | ||
| func GenerateRHOKPWaitInitContainer(image, namespace string) corev1.Container { | ||
| rhokpURL := fmt.Sprintf("https://%s.%s.svc:%d/solr/portal-rag/select?q=*:*&rows=0&wt=json", |
There was a problem hiding this comment.
Readiness endpoint mismatch + duplicated URL construction
Two issues on this line:
-
Endpoint: This polls
/solr/portal-rag/select?q=*:*&rows=0&wt=json, but RHOKP's own readiness/startup probes useRHOOKPReadinessHTTPPath=/solr/portal-rag/admin/ping. Theselectendpoint is heavier — it queries the Solr index (even withrows=0). During startup,/admin/pingbecomes available first. The init container should use the same endpoint as the readiness probe. -
URL helper: There's an existing
RHOKPServiceURL(namespace)helper (constants.go:714) that returnshttps://lightspeed-rhokp.<namespace>.svc:8443. This could be reused:
rhokpURL := RHOKPServiceURL(namespace) + RHOOKPReadinessHTTPPathThere was a problem hiding this comment.
Fixed — now uses RHOKPServiceURL(namespace) + RHOOKPReadinessHTTPPath (/solr/portal-rag/admin/ping). Lighter endpoint, available earlier during startup, and no duplicated URL construction.
When ByokRAGOnly=false the operator now adds a wait-for-rhokp init container that polls RHOKP/Solr until it responds, preventing the app-server from starting before Solr is reachable. Without this gate the SolrHybridSearch @cached_property can permanently cache None if RHOKP takes longer than the 120 s retry window. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vimal Kumar <vimal78@gmail.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
Fixed — switched from cr.Namespace to r.GetNamespace() which returns the operator watch namespace. Kept the FQDN approach (with namespace) rather than short service name because the service-ca TLS cert SANs use the FQDN — curl --cacert would fail cert validation against a short hostname.
|
Re @blublinsky review — fixed. Switched from |
|
/lgtm |
|
the fix to lightspeed service as @blublinsky suggested can be addressed in another PR to improve resiliancy of that component. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raptorsun 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
|
@vimalk78: 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
3 participants
Summary
wait-for-rhokpinit container to the app-server Deployment whenByokRAGOnly=falseSolrHybridSearch's@cached_propertypermanently cachesNoneif RHOKP takes longer than the 120s retry windowwait-for-postgresinit container patternTest plan
GenerateRHOKPWaitInitContainer(image, security context, CA mount, curl command, timeout, namespace)ByokRAGOnly=false, absent whenByokRAGOnly=trueassets_test.goConsistOfassertion updated for the new init containermake testpasses (130/130 specs)ByokRAGOnly=false, verify init container blocks app-server startup until RHOKP is serving🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes