OLS-3450: Add credentialHotReload flag to skip app-server restart on LLM secret rotation#1779
OLS-3450: Add credentialHotReload flag to skip app-server restart on LLM secret rotation#1779lalan7 wants to merge 1 commit into
Conversation
…LLM secret rotation Adds an opt-in credentialHotReload boolean to OLSSpec CRD. When enabled, the operator's secret watcher skips rolling restart for LLM credential secrets, allowing the service's in-process hot-reload to handle rotated tokens without downtime (RFE-9380). Default: false (preserves existing behavior). - New CRD field: spec.olsConfig.credentialHotReload (default: false) - Watcher: skip restart for LLM secrets when flag is true - Info log emitted during reconciliation when enabled - Unit tests (3 new) + e2e test for hot-reload skip - Documentation: docs/credential-hot-reload.md Companion to lightspeed-service PR #2955.
openshift-ci-robot
added
the
jira/valid-reference
|
@lalan7: This pull request references OLS-3450 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. |
📝 WalkthroughWalkthroughAdds a ChangesCredential Hot-Reload
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler as OLSConfigReconciler
participant Watcher as WatcherConfig
participant Filter as SecretWatcherFilter
participant Deployment as AppServerDeployment
Reconciler->>Watcher: set CredentialHotReload, populate LLMSecretNames
Note over Filter: On secret change event
Filter->>Watcher: check CredentialHotReload and LLMSecretNames
alt hot-reload enabled and secret tracked
Filter-->>Filter: log skip, no restart
else
Filter->>Deployment: annotate for restart
end
PoemA rabbit hops through secrets deep, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/watchers/watchers_test.go (1)
363-471: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated Deployment/reconciler setup into a helper.
The three new tests duplicate identical
dep/createTestReconcilerboilerplate. A small local helper (e.g.,newTestDeploymentAndReconciler(cr)) would reduce repetition within this file.🤖 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_test.go` around lines 363 - 471, The new Credential hot-reload tests duplicate the same Deployment and reconciler setup in each case. Extract that repeated boilerplate into a small local helper in watchers_test.go, such as a helper that builds the standard Deployment and calls createTestReconciler, then reuse it in the three Describe/It blocks while keeping the test-specific watcher config and Secret setup inline.
🤖 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/types.go`:
- Around line 60-73: `WatcherConfig` is shared mutable state and is accessed
concurrently by `SecretWatcherFilter()` while `annotateExternalResources()`
updates `CredentialHotReload` and `LLMSecretNames`; add synchronization or
switch to an immutable snapshot approach. Update the `WatcherConfig` type in
`types.go` to guard these fields with a mutex (or refactor to store/retrieve a
copied config atomically), and make both `SecretWatcherFilter()` and
`annotateExternalResources()` use the same protection when reading or writing
`CredentialHotReload` and `LLMSecretNames`.
In `@test/e2e/reconciliation_test.go`:
- Around line 400-416: The reconciliation test uses a fixed sleep after updating
OLSConfig.CredentialHotReload, which makes the hot-reload setup timing-dependent
and flaky. Replace the blind time.Sleep in the credentialHotReload setup with an
Eventually-based wait on an observable signal from the reconciler, such as a
status condition, annotation, or other field updated when the latest
spec.generation has been processed. Use the existing client.Update, client.Get,
and deployment.Generation flow to locate the test, and keep the later
Consistently assertion only after the reconciler-ready signal is observed.
---
Nitpick comments:
In `@internal/controller/watchers/watchers_test.go`:
- Around line 363-471: The new Credential hot-reload tests duplicate the same
Deployment and reconciler setup in each case. Extract that repeated boilerplate
into a small local helper in watchers_test.go, such as a helper that builds the
standard Deployment and calls createTestReconciler, then reuse it in the three
Describe/It blocks while keeping the test-specific watcher config and Secret
setup inline.
🪄 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: Enterprise
Run ID: b0d1bbf1-2bfc-49b1-a705-c12229c81079
⛔ Files ignored due to path filters (1)
config/crd/bases/ols.openshift.io_olsconfigs.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (8)
api/v1alpha1/olsconfig_types.godocs/credential-hot-reload.mdinternal/controller/olsconfig_controller.gointernal/controller/olsconfig_helpers.gointernal/controller/utils/types.gointernal/controller/watchers/watchers.gointernal/controller/watchers/watchers_test.gotest/e2e/reconciliation_test.go
| // WatcherConfig contains all watcher configuration. | ||
| // NOTE: This struct is written by the reconciler and read by watcher event handlers | ||
| // (including predicate filters such as SecretWatcherFilter). This is safe because | ||
| // controller-runtime serializes reconcile calls and predicate evaluations on the same | ||
| // controller work queue. If MaxConcurrentReconciles is ever increased above 1, | ||
| // a sync.RWMutex must be added here. | ||
| type WatcherConfig struct { | ||
| Secrets SecretWatcherConfig | ||
| ConfigMaps ConfigMapWatcherConfig | ||
| AnnotatedSecretMapping map[string][]string | ||
| AnnotatedConfigMapMapping map[string][]string | ||
| CredentialHotReload bool | ||
| LLMSecretNames map[string]bool | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does controller-runtime call Predicate.Update/Create in the same goroutine as Reconcile, or in the informer event handler goroutine?
💡 Result:
In controller-runtime, Predicate functions (Create, Update, Delete, Generic) are executed in the informer event handler goroutine, not in the goroutine that runs the Reconcile function [1][2]. Predicates operate within the informer event path, where they serve as filters that determine whether an event should be enqueued for reconciliation [1][3]. Because they execute before a request enters the workqueue, they must be efficient, non-blocking, and should not perform expensive computations or API calls [1]. In contrast, the Reconcile function runs in a separate pool of worker goroutines dedicated to processing the workqueue [1][4]. The number of concurrent Reconcile goroutines is controlled by the MaxConcurrentReconciles configuration [1]. The separation ensures that the critical path of informer event processing remains fast, preventing bottlenecks from propagating back to the informer's event stream [1].
Citations:
- 1: https://www.golinuxcloud.com/controller-runtime-performance-tuning/
- 2: Events for Owned objects will bypass predicates for the controlled type kubernetes-sigs/controller-runtime#2007
- 3: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/predicate
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/doc.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and find all uses of WatcherConfig / LLMSecretNames / CredentialHotReload.
ast-grep outline internal/controller/utils/types.go --view expanded || true
printf '\n--- uses ---\n'
rg -n "WatcherConfig|LLMSecretNames|CredentialHotReload|SecretWatcherFilter|annotateExternalResources" internal/controller -S
printf '\n--- types.go excerpt ---\n'
sed -n '1,160p' internal/controller/utils/types.go
printf '\n--- relevant controller excerpts ---\n'
for f in $(rg -l "WatcherConfig|LLMSecretNames|CredentialHotReload|SecretWatcherFilter|annotateExternalResources" internal/controller -S); do
echo "### $f"
sed -n '1,260p' "$f"
doneRepository: openshift/lightspeed-operator
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- watcher filter ---\n'
sed -n '220,340p' internal/controller/watchers/watchers.go
printf '\n--- annotateExternalResources ---\n'
sed -n '360,420p' internal/controller/olsconfig_helpers.go
printf '\n--- any synchronization around WatcherConfig ---\n'
rg -n "RWMutex|Mutex|Lock\\(|RLock\\(|WatcherConfig" internal/controller -S
printf '\n--- setup/watch registration ---\n'
sed -n '520,700p' internal/controller/olsconfig_controller.goRepository: openshift/lightspeed-operator
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- watcher filter ---'
sed -n '220,340p' internal/controller/watchers/watchers.go
echo
echo '--- annotateExternalResources ---'
sed -n '360,420p' internal/controller/olsconfig_helpers.go
echo
echo '--- synchronization search ---'
rg -n "RWMutex|Mutex|Lock\\(|RLock\\(|WatcherConfig" internal/controller -S
echo
echo '--- watch registration ---'
sed -n '520,700p' internal/controller/olsconfig_controller.goRepository: openshift/lightspeed-operator
Length of output: 21680
Protect WatcherConfig from concurrent access. SecretWatcherFilter() reads CredentialHotReload and LLMSecretNames while annotateExternalResources() rewrites them during reconcile on a different goroutine. This shared state needs a mutex or immutable snapshot; otherwise a secret event can race with reconcile and hit concurrent map access.
🤖 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/utils/types.go` around lines 60 - 73, `WatcherConfig` is
shared mutable state and is accessed concurrently by `SecretWatcherFilter()`
while `annotateExternalResources()` updates `CredentialHotReload` and
`LLMSecretNames`; add synchronization or switch to an immutable snapshot
approach. Update the `WatcherConfig` type in `types.go` to guard these fields
with a mutex (or refactor to store/retrieve a copied config atomically), and
make both `SecretWatcherFilter()` and `annotateExternalResources()` use the same
protection when reading or writing `CredentialHotReload` and `LLMSecretNames`.
| By("enable credentialHotReload on the CR") | ||
| err = client.Update(cr, func(obj ctrlclient.Object) error { | ||
| config := obj.(*olsv1alpha1.OLSConfig) | ||
| hotReload := true | ||
| config.Spec.OLSConfig.CredentialHotReload = &hotReload | ||
| return nil | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // credentialHotReload only affects in-memory WatcherConfig, not the | ||
| // Deployment spec, so there is no Generation bump to wait for. | ||
| // Give the reconciler time to process the CR update. | ||
| time.Sleep(5 * time.Second) | ||
|
|
||
| err = client.Get(deployment) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| generation := deployment.Generation |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Hard-coded sleep for reconciliation is a flakiness risk.
time.Sleep(5 * time.Second) assumes the reconciler will have processed the credentialHotReload CR update (and repopulated WatcherConfig) within 5 seconds. Under CI load or slower reconcile loops, the LLM secret rotation could race ahead of the watcher-config update, causing the subsequent Consistently check to intermittently fail (the deployment would restart because hot-reload wasn't yet active in-memory).
Since there's no Deployment/ConfigMap side-effect to poll on (per the comment), consider polling on an observable signal instead of a blind sleep — e.g., increasing the sleep with a retry/backoff, or exposing a lightweight status condition/annotation that reflects the reconciler having processed the latest spec.generation, and using Eventually against that.
💡 Possible mitigation without a new status signal
- // credentialHotReload only affects in-memory WatcherConfig, not the
- // Deployment spec, so there is no Generation bump to wait for.
- // Give the reconciler time to process the CR update.
- time.Sleep(5 * time.Second)
+ // credentialHotReload only affects in-memory WatcherConfig, not the
+ // Deployment spec, so there is no Generation bump to wait for.
+ // Poll to reduce flakiness risk vs. a fixed sleep.
+ Eventually(func() bool {
+ var current olsv1alpha1.OLSConfig
+ current.Name = cr.Name
+ if err := client.Get(¤t); err != nil {
+ return false
+ }
+ return current.Spec.OLSConfig.CredentialHotReload != nil && *current.Spec.OLSConfig.CredentialHotReload
+ }, 15*time.Second, 1*time.Second).Should(BeTrue())As per path instructions, "Flag flaky timing, missing teardown, or assumptions about optional components not deployed in bundle/operator CI jobs."
🤖 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 `@test/e2e/reconciliation_test.go` around lines 400 - 416, The reconciliation
test uses a fixed sleep after updating OLSConfig.CredentialHotReload, which
makes the hot-reload setup timing-dependent and flaky. Replace the blind
time.Sleep in the credentialHotReload setup with an Eventually-based wait on an
observable signal from the reconciler, such as a status condition, annotation,
or other field updated when the latest spec.generation has been processed. Use
the existing client.Update, client.Get, and deployment.Generation flow to locate
the test, and keep the later Consistently assertion only after the
reconciler-ready signal is observed.
Source: Path instructions
|
@lalan7: The following test failed, say
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. |
|
Hi @blublinsky — this is the operator companion to service PR openshift/lightspeed-service#2955 (RFE-9380). Adds an opt-in All CI checks pass. The Would appreciate a review when you have a chance. |
|
Suggestion: drive hot-reload from The flag is currently wired in two places: A simpler approach is to handle this inside the When the flag is off, behavior stays as today: foreach annotates LLM credential secrets and rotations trigger a rolling restart as they do now. That keeps “which secrets does the operator react to?” entirely in the existing foreach/mapping configuration model, instead of splitting it between reconcile-time config and filter-time exceptions. One thing to nail when toggling the flag on for clusters that already have annotated LLM secrets: reconcile may need to remove the watcher annotation when hot-reload is enabled, not only skip adding it for new ones. I’d hold off on deeper review until this is reworked — the foreach-based approach removes most of the PR’s watcher-layer changes and is the pattern we already use for other secret types. |
|
Follow-up: Today this flag lives only on the CR and in operator That leaves two independent behaviors with no runtime coupling:
For this to work end-to-end, the flag should flow: Without that, enabling the CR flag on a cluster with an older service image (or before #2955 is deployed) silently disables restarts while credentials stay stale at startup — the CR comment warns about this, but nothing in config enforces it. Suggest:
That gives one user-facing switch that coordinates both “don’t restart” and “do re-read from disk,” instead of two repos assuming the other half is always correct. |
|
Scoping: operator-only PR + bundle follow-up This PR is correctly scoped to the operator only — service changes stay in openshift/lightspeed-service#2955. Keep it that way. Once the operator changes here are in good shape, suggest landing this PR without blocking on the OLM bundle, then a follow-up PR that runs Order I’d expect:
That keeps review focused and matches how we usually ship CRD schema vs bundle image updates. |
3 participants
Summary
credentialHotReloadboolean to OLSSpec CRD (default:false)Test plan
Consistentlygo vet ./...cleango build ./...cleanmake manifestsRelated