-
Notifications
You must be signed in to change notification settings - Fork 52
OLS-3450: Add credentialHotReload flag to skip app-server restart on LLM secret rotation #1779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # Credential Hot-Reload (RFE-9380) | ||
|
|
||
| ## Overview | ||
|
|
||
| When `credentialHotReload` is enabled on the OLSConfig CR, the operator skips | ||
| rolling restarts of the app-server pod when LLM credential secret **data** | ||
| changes. The service re-reads credential files from disk on every LLM request, | ||
| so rotated tokens take effect without downtime. | ||
|
|
||
| This feature is a companion to | ||
| [lightspeed-service PR #2955](https://github.com/openshift/lightspeed-service/pull/2955), | ||
| which adds the in-process credential reload on the service side. | ||
|
|
||
| ## Configuration | ||
|
|
||
| ```yaml | ||
| apiVersion: ols.openshift.io/v1alpha1 | ||
| kind: OLSConfig | ||
| metadata: | ||
| name: cluster | ||
| spec: | ||
| ols: | ||
| credentialHotReload: true # default: false | ||
| ``` | ||
|
|
||
| ## Behavior | ||
|
|
||
| | Event | `credentialHotReload: false` (default) | `credentialHotReload: true` | | ||
| |---|---|---| | ||
| | LLM secret **data** rotated (same secret name) | Rolling restart | **No restart** — service picks up new credentials on next request | | ||
| | LLM secret **ref** changed in CR (different secret name) | Rolling restart | Rolling restart (deployment volume spec changes) | | ||
| | TLS / MCP / Postgres secret changed | Rolling restart | Rolling restart (unchanged) | | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| - The lightspeed-service image must include the `get_credentials()` hot-reload | ||
| support (lightspeed-service >= the version containing PR #2955). If an older | ||
| service image is used with this flag enabled, rotated credentials will not be | ||
| picked up until the pod is manually restarted. | ||
|
|
||
| ## How It Works | ||
|
|
||
| 1. During reconciliation, the operator reads `spec.ols.credentialHotReload` from | ||
| the OLSConfig CR and stores it in the internal `WatcherConfig`, along with the | ||
| set of LLM provider secret names. | ||
|
|
||
| 2. When the secret watcher detects a `.data` change on an annotated secret, it | ||
| checks whether the secret is an LLM credential and the hot-reload flag is | ||
| enabled. If both conditions are true, the restart is skipped. | ||
|
|
||
| 3. Non-LLM secrets (TLS, MCP headers, Postgres) always trigger restarts | ||
| regardless of the flag. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -389,4 +389,59 @@ var _ = Describe("Reconciliation From OLSConfig CR", Ordered, func() { | |
|
|
||
| }) | ||
|
|
||
| It("should skip app-server restart on LLM secret data rotation when credentialHotReload is enabled", func() { | ||
| deployment := &appsv1.Deployment{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: AppServerDeploymentName, | ||
| Namespace: OLSNameSpace, | ||
| }, | ||
| } | ||
|
|
||
| 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 | ||
|
Comment on lines
+400
to
+416
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Hard-coded sleep for reconciliation is a flakiness risk.
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 💡 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 AgentsSource: Path instructions |
||
|
|
||
| By("rotate the LLM secret data (using second secret — CR was switched to it by an earlier test)") | ||
| llmSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: LLMTokenSecondSecretName, | ||
| Namespace: OLSNameSpace, | ||
| }, | ||
| } | ||
| err = client.Update(llmSecret, func(obj ctrlclient.Object) error { | ||
| s := obj.(*corev1.Secret) | ||
| s.Data[LLMApiTokenFileName] = []byte("rotated-key-value") | ||
| return nil | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("verify the deployment generation did NOT change (no restart)") | ||
| Consistently(func() int64 { | ||
| Expect(client.Get(deployment)).To(Succeed()) | ||
| return deployment.Generation | ||
| }, 15*time.Second, 2*time.Second).Should(Equal(generation)) | ||
|
|
||
| By("disable credentialHotReload to restore default behavior") | ||
| err = client.Update(cr, func(obj ctrlclient.Object) error { | ||
| config := obj.(*olsv1alpha1.OLSConfig) | ||
| config.Spec.OLSConfig.CredentialHotReload = nil | ||
| return nil | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 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:
🏁 Script executed:
Repository: openshift/lightspeed-operator
Length of output: 50386
🏁 Script executed:
Repository: openshift/lightspeed-operator
Length of output: 258
🏁 Script executed:
Repository: openshift/lightspeed-operator
Length of output: 21680
Protect
WatcherConfigfrom concurrent access.SecretWatcherFilter()readsCredentialHotReloadandLLMSecretNameswhileannotateExternalResources()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