Skip to content

Commit 67d06e2

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1448 from blublinsky/mcp-config
Track MCP Server ConfigMap ResourceVersion for Pod Restarts
2 parents c951c4e + bfecc2c commit 67d06e2

5 files changed

Lines changed: 353 additions & 16 deletions

File tree

‎internal/controller/appserver/deployment.go‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"github.com/openshift/lightspeed-operator/internal/controller/utils"
2424
)
2525

26-
2726
func getOLSServerResources(cr *olsv1alpha1.OLSConfig) *corev1.ResourceRequirements {
2827
return utils.GetResourcesOrDefault(
2928
cr.Spec.OLSConfig.DeploymentConfig.APIContainer.Resources,
@@ -367,13 +366,19 @@ func GenerateOLSDeployment(r reconciler.Reconciler, cr *olsv1alpha1.OLSConfig) (
367366
return nil, fmt.Errorf("failed to get ConfigMap resource version: %w", err)
368367
}
369368

369+
mcpConfigMapResourceVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.OpenShiftMCPServerConfigCmName)
370+
if err != nil && !apierrors.IsNotFound(err) {
371+
return nil, fmt.Errorf("failed to get MCP Server ConfigMap resource version: %w", err)
372+
}
373+
370374
deployment := appsv1.Deployment{
371375
ObjectMeta: metav1.ObjectMeta{
372376
Name: utils.OLSAppServerDeploymentName,
373377
Namespace: r.GetNamespace(),
374378
Labels: utils.GenerateAppServerSelectorLabels(),
375379
Annotations: map[string]string{
376-
utils.OLSConfigMapResourceVersionAnnotation: configMapResourceVersion,
380+
utils.OLSConfigMapResourceVersionAnnotation: configMapResourceVersion,
381+
utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation: mcpConfigMapResourceVersion,
377382
},
378383
},
379384
Spec: appsv1.DeploymentSpec{
@@ -496,7 +501,7 @@ func GenerateOLSDeployment(r reconciler.Reconciler, cr *olsv1alpha1.OLSConfig) (
496501
Image: r.GetOpenShiftMCPServerImage(),
497502
ImagePullPolicy: corev1.PullIfNotPresent,
498503
SecurityContext: utils.RestrictedContainerSecurityContext(),
499-
VolumeMounts: []corev1.VolumeMount{configMount},
504+
VolumeMounts: []corev1.VolumeMount{configMount},
500505
Command: []string{
501506
"/openshift-mcp-server",
502507
"--read-only",
@@ -530,6 +535,19 @@ func updateOLSDeployment(r reconciler.Reconciler, ctx context.Context, existingD
530535
}
531536
}
532537

538+
// Step 3: Check if MCP Server ConfigMap ResourceVersion has changed
539+
currentMCPConfigMapVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.OpenShiftMCPServerConfigCmName)
540+
if err != nil && !apierrors.IsNotFound(err) {
541+
r.GetLogger().Info("failed to get MCP Server ConfigMap ResourceVersion", "error", err)
542+
changed = true
543+
} else {
544+
storedMCPConfigMapVersion := existingDeployment.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
545+
if storedMCPConfigMapVersion != currentMCPConfigMapVersion {
546+
r.GetLogger().Info("MCP Server ConfigMap changed, updating deployment")
547+
changed = true
548+
}
549+
}
550+
533551
// If nothing changed, skip update
534552
if !changed {
535553
return nil
@@ -538,6 +556,7 @@ func updateOLSDeployment(r reconciler.Reconciler, ctx context.Context, existingD
538556
// Apply changes - always update spec and annotations since something changed
539557
existingDeployment.Spec = desiredDeployment.Spec
540558
existingDeployment.Annotations[utils.OLSConfigMapResourceVersionAnnotation] = desiredDeployment.Annotations[utils.OLSConfigMapResourceVersionAnnotation]
559+
existingDeployment.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation] = desiredDeployment.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
541560

542561
r.GetLogger().Info("updating OLS deployment", "name", existingDeployment.Name)
543562

‎internal/controller/appserver/reconciler_test.go‎

Lines changed: 129 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import (
1515
corev1 "k8s.io/api/core/v1"
1616
networkingv1 "k8s.io/api/networking/v1"
1717
rbacv1 "k8s.io/api/rbac/v1"
18-
"k8s.io/apimachinery/pkg/api/errors"
18+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1919
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2020
"k8s.io/apimachinery/pkg/types"
2121
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -219,6 +219,132 @@ var _ = Describe("App server reconciliator", Ordered, func() {
219219
Expect(dep.Spec.Template.Spec.Tolerations).To(Equal(olsConfig.Spec.OLSConfig.DeploymentConfig.APIContainer.Tolerations))
220220
})
221221

222+
It("should track MCP Server ConfigMap ResourceVersion and trigger restart on introspection toggle", func() {
223+
By("Disable introspection initially")
224+
olsConfig := &olsv1alpha1.OLSConfig{}
225+
err := k8sClient.Get(ctx, crNamespacedName, olsConfig)
226+
Expect(err).NotTo(HaveOccurred())
227+
olsConfig.Spec.OLSConfig.IntrospectionEnabled = false
228+
229+
By("Reconcile with introspection disabled")
230+
err = ReconcileAppServer(testReconcilerInstance, ctx, olsConfig)
231+
Expect(err).NotTo(HaveOccurred())
232+
233+
By("Verify MCP Server ConfigMap does not exist")
234+
mcpCm := &corev1.ConfigMap{}
235+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OpenShiftMCPServerConfigCmName, Namespace: utils.OLSNamespaceDefault}, mcpCm)
236+
Expect(apierrors.IsNotFound(err)).To(BeTrue(), "MCP ConfigMap should not exist when introspection is disabled")
237+
238+
By("Get deployment and verify MCP annotation is empty")
239+
dep := &appsv1.Deployment{}
240+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OLSAppServerDeploymentName, Namespace: utils.OLSNamespaceDefault}, dep)
241+
Expect(err).NotTo(HaveOccurred())
242+
mcpAnnotation := dep.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
243+
Expect(mcpAnnotation).To(BeEmpty(), "MCP annotation should be empty when ConfigMap doesn't exist")
244+
245+
By("Enable introspection")
246+
err = k8sClient.Get(ctx, crNamespacedName, olsConfig)
247+
Expect(err).NotTo(HaveOccurred())
248+
olsConfig.Spec.OLSConfig.IntrospectionEnabled = true
249+
250+
By("Reconcile with introspection enabled")
251+
err = ReconcileAppServer(testReconcilerInstance, ctx, olsConfig)
252+
Expect(err).NotTo(HaveOccurred())
253+
254+
By("Verify MCP Server ConfigMap was created")
255+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OpenShiftMCPServerConfigCmName, Namespace: utils.OLSNamespaceDefault}, mcpCm)
256+
Expect(err).NotTo(HaveOccurred(), "MCP ConfigMap should exist after enabling introspection")
257+
firstResourceVersion := mcpCm.ResourceVersion
258+
Expect(firstResourceVersion).NotTo(BeEmpty())
259+
260+
By("Verify deployment annotation tracks MCP ConfigMap ResourceVersion")
261+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OLSAppServerDeploymentName, Namespace: utils.OLSNamespaceDefault}, dep)
262+
Expect(err).NotTo(HaveOccurred())
263+
mcpAnnotation = dep.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
264+
Expect(mcpAnnotation).To(Equal(firstResourceVersion), "Deployment annotation should match ConfigMap ResourceVersion")
265+
266+
By("Disable introspection again")
267+
err = k8sClient.Get(ctx, crNamespacedName, olsConfig)
268+
Expect(err).NotTo(HaveOccurred())
269+
olsConfig.Spec.OLSConfig.IntrospectionEnabled = false
270+
271+
By("Reconcile with introspection disabled again")
272+
err = ReconcileAppServer(testReconcilerInstance, ctx, olsConfig)
273+
Expect(err).NotTo(HaveOccurred())
274+
275+
By("Verify MCP Server ConfigMap was deleted")
276+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OpenShiftMCPServerConfigCmName, Namespace: utils.OLSNamespaceDefault}, mcpCm)
277+
Expect(apierrors.IsNotFound(err)).To(BeTrue(), "MCP ConfigMap should be deleted when introspection is disabled")
278+
279+
By("Verify deployment annotation changed (triggering restart)")
280+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OLSAppServerDeploymentName, Namespace: utils.OLSNamespaceDefault}, dep)
281+
Expect(err).NotTo(HaveOccurred())
282+
newMcpAnnotation := dep.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
283+
Expect(newMcpAnnotation).NotTo(Equal(firstResourceVersion), "Annotation should change when ConfigMap is deleted to trigger pod restart")
284+
285+
By("Verify pod template restart annotation is set")
286+
forceReloadValue := dep.Spec.Template.Annotations[utils.ForceReloadAnnotationKey]
287+
Expect(forceReloadValue).NotTo(BeEmpty(), "Pod template restart annotation should be set to trigger rolling update")
288+
})
289+
290+
It("should trigger rolling update when MCP ConfigMap is modified externally", func() {
291+
By("Enable introspection")
292+
olsConfig := &olsv1alpha1.OLSConfig{}
293+
err := k8sClient.Get(ctx, crNamespacedName, olsConfig)
294+
Expect(err).NotTo(HaveOccurred())
295+
olsConfig.Spec.OLSConfig.IntrospectionEnabled = true
296+
297+
By("Reconcile to create MCP ConfigMap")
298+
err = ReconcileAppServer(testReconcilerInstance, ctx, olsConfig)
299+
Expect(err).NotTo(HaveOccurred())
300+
301+
By("Get the MCP ConfigMap and capture initial ResourceVersion")
302+
mcpCm := &corev1.ConfigMap{}
303+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OpenShiftMCPServerConfigCmName, Namespace: utils.OLSNamespaceDefault}, mcpCm)
304+
Expect(err).NotTo(HaveOccurred())
305+
initialResourceVersion := mcpCm.ResourceVersion
306+
307+
By("Get deployment and capture initial annotation")
308+
dep := &appsv1.Deployment{}
309+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OLSAppServerDeploymentName, Namespace: utils.OLSNamespaceDefault}, dep)
310+
Expect(err).NotTo(HaveOccurred())
311+
initialAnnotation := dep.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
312+
Expect(initialAnnotation).To(Equal(initialResourceVersion))
313+
314+
By("Manually modify MCP ConfigMap data (simulating external change)")
315+
mcpCm.Data["mcp-server-config.toml"] = "# Modified externally"
316+
err = k8sClient.Update(ctx, mcpCm)
317+
Expect(err).NotTo(HaveOccurred())
318+
319+
By("Get updated ResourceVersion after manual modification")
320+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OpenShiftMCPServerConfigCmName, Namespace: utils.OLSNamespaceDefault}, mcpCm)
321+
Expect(err).NotTo(HaveOccurred())
322+
modifiedResourceVersion := mcpCm.ResourceVersion
323+
Expect(modifiedResourceVersion).NotTo(Equal(initialResourceVersion), "ResourceVersion should change after update")
324+
325+
By("Reconcile again to correct the ConfigMap")
326+
err = ReconcileAppServer(testReconcilerInstance, ctx, olsConfig)
327+
Expect(err).NotTo(HaveOccurred())
328+
329+
By("Verify ConfigMap was corrected and has new ResourceVersion")
330+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OpenShiftMCPServerConfigCmName, Namespace: utils.OLSNamespaceDefault}, mcpCm)
331+
Expect(err).NotTo(HaveOccurred())
332+
correctedResourceVersion := mcpCm.ResourceVersion
333+
Expect(correctedResourceVersion).NotTo(Equal(modifiedResourceVersion), "ResourceVersion should change after reconciler correction")
334+
Expect(mcpCm.Data["mcp-server-config.toml"]).NotTo(ContainSubstring("Modified externally"), "ConfigMap should be corrected to proper content")
335+
336+
By("Verify deployment annotation updated to new ResourceVersion")
337+
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.OLSAppServerDeploymentName, Namespace: utils.OLSNamespaceDefault}, dep)
338+
Expect(err).NotTo(HaveOccurred())
339+
newAnnotation := dep.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
340+
Expect(newAnnotation).To(Equal(correctedResourceVersion), "Deployment annotation should track corrected ConfigMap ResourceVersion")
341+
Expect(newAnnotation).NotTo(Equal(initialAnnotation), "Deployment annotation should change to trigger restart")
342+
343+
By("Verify pod template restart annotation is set")
344+
forceReloadValue := dep.Spec.Template.Annotations[utils.ForceReloadAnnotationKey]
345+
Expect(forceReloadValue).NotTo(BeEmpty(), "Pod template restart annotation should be set to trigger rolling update")
346+
})
347+
222348
It("should trigger rolling update of the deployment when updating the nodeselector ", func() {
223349
By("Get the deployment")
224350
dep := &appsv1.Deployment{}
@@ -413,7 +539,7 @@ var _ = Describe("App server reconciliator", Ordered, func() {
413539

414540
By("Verify exporter configmap has been deleted")
415541
err = k8sClient.Get(ctx, types.NamespacedName{Name: utils.ExporterConfigCmName, Namespace: utils.OLSNamespaceDefault}, &corev1.ConfigMap{})
416-
Expect(errors.IsNotFound(err)).To(BeTrue())
542+
Expect(apierrors.IsNotFound(err)).To(BeTrue())
417543
})
418544

419545
// Note: LLM credential validation is now done in annotateExternalResources
@@ -473,7 +599,7 @@ var _ = Describe("App server reconciliator", Ordered, func() {
473599
By("Delete the tls secret")
474600
secretDeletionErr = testReconcilerInstance.Delete(ctx, tlsSecret)
475601
if secretDeletionErr != nil {
476-
Expect(errors.IsNotFound(secretDeletionErr)).To(BeTrue())
602+
Expect(apierrors.IsNotFound(secretDeletionErr)).To(BeTrue())
477603
} else {
478604
Expect(secretDeletionErr).NotTo(HaveOccurred())
479605
}

‎internal/controller/lcore/deployment.go‎

Lines changed: 49 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -711,9 +711,20 @@ func generateLCoreServerDeployment(r reconciler.Reconciler, ctx context.Context,
711711
lightspeedStackResources := getLightspeedStackResources(cr)
712712

713713
// Get ResourceVersions for tracking - these resources should already exist
714-
// If they don't exist, we'll get empty strings which is fine for initial creation
715-
lcoreConfigMapResourceVersion, _ := utils.GetConfigMapResourceVersion(r, ctx, utils.LCoreConfigCmName)
716-
llamaStackConfigMapResourceVersion, _ := utils.GetConfigMapResourceVersion(r, ctx, utils.LlamaStackConfigCmName)
714+
// If they don't exist (NotFound), we'll get empty strings which is fine for initial creation
715+
// However, we should not ignore other errors (like API failures)
716+
lcoreConfigMapResourceVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.LCoreConfigCmName)
717+
if err != nil && !apierrors.IsNotFound(err) {
718+
return nil, fmt.Errorf("failed to get LCore ConfigMap resource version: %w", err)
719+
}
720+
llamaStackConfigMapResourceVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.LlamaStackConfigCmName)
721+
if err != nil && !apierrors.IsNotFound(err) {
722+
return nil, fmt.Errorf("failed to get Llama Stack ConfigMap resource version: %w", err)
723+
}
724+
mcpConfigMapResourceVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.OpenShiftMCPServerConfigCmName)
725+
if err != nil && !apierrors.IsNotFound(err) {
726+
return nil, fmt.Errorf("failed to get MCP Server ConfigMap resource version: %w", err)
727+
}
717728

718729
// Use helper functions to build common components
719730
labels := buildCommonLabels()
@@ -908,8 +919,9 @@ func generateLCoreServerDeployment(r reconciler.Reconciler, ctx context.Context,
908919
Namespace: r.GetNamespace(),
909920
Labels: labels,
910921
Annotations: map[string]string{
911-
utils.LCoreConfigMapResourceVersionAnnotation: lcoreConfigMapResourceVersion,
912-
utils.LlamaStackConfigMapResourceVersionAnnotation: llamaStackConfigMapResourceVersion,
922+
utils.LCoreConfigMapResourceVersionAnnotation: lcoreConfigMapResourceVersion,
923+
utils.LlamaStackConfigMapResourceVersionAnnotation: llamaStackConfigMapResourceVersion,
924+
utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation: mcpConfigMapResourceVersion,
913925
},
914926
},
915927
Spec: appsv1.DeploymentSpec{
@@ -1029,6 +1041,19 @@ func updateLCoreDeployment(r reconciler.Reconciler, ctx context.Context, existin
10291041
}
10301042
}
10311043

1044+
// Check if MCP Server ConfigMap ResourceVersion has changed
1045+
currentMCPConfigMapVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.OpenShiftMCPServerConfigCmName)
1046+
if err != nil && !apierrors.IsNotFound(err) {
1047+
r.GetLogger().Info("failed to get MCP Server ConfigMap ResourceVersion", "error", err)
1048+
changed = true
1049+
} else {
1050+
storedMCPConfigMapVersion := existingDeployment.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
1051+
if storedMCPConfigMapVersion != currentMCPConfigMapVersion {
1052+
r.GetLogger().Info("MCP Server ConfigMap changed, updating deployment")
1053+
changed = true
1054+
}
1055+
}
1056+
10321057
// If nothing changed, skip update
10331058
if !changed {
10341059
return nil
@@ -1038,6 +1063,7 @@ func updateLCoreDeployment(r reconciler.Reconciler, ctx context.Context, existin
10381063
existingDeployment.Spec = desiredDeployment.Spec
10391064
existingDeployment.Annotations[utils.LCoreConfigMapResourceVersionAnnotation] = desiredDeployment.Annotations[utils.LCoreConfigMapResourceVersionAnnotation]
10401065
existingDeployment.Annotations[utils.LlamaStackConfigMapResourceVersionAnnotation] = desiredDeployment.Annotations[utils.LlamaStackConfigMapResourceVersionAnnotation]
1066+
existingDeployment.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation] = desiredDeployment.Annotations[utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation]
10411067

10421068
r.GetLogger().Info("updating LCore deployment", "name", existingDeployment.Name)
10431069

@@ -1059,9 +1085,21 @@ func generateLCoreLibraryDeployment(r reconciler.Reconciler, ctx context.Context
10591085
return nil, fmt.Errorf("failed to check data collector status: %w", err)
10601086
}
10611087

1062-
// Get ResourceVersions for tracking
1063-
lcoreConfigMapResourceVersion, _ := utils.GetConfigMapResourceVersion(r, ctx, utils.LCoreConfigCmName)
1064-
llamaStackConfigMapResourceVersion, _ := utils.GetConfigMapResourceVersion(r, ctx, utils.LlamaStackConfigCmName)
1088+
// Get ResourceVersions for tracking - these resources should already exist
1089+
// If they don't exist (NotFound), we'll get empty strings which is fine for initial creation
1090+
// However, we should not ignore other errors (like API failures)
1091+
lcoreConfigMapResourceVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.LCoreConfigCmName)
1092+
if err != nil && !apierrors.IsNotFound(err) {
1093+
return nil, fmt.Errorf("failed to get LCore ConfigMap resource version: %w", err)
1094+
}
1095+
llamaStackConfigMapResourceVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.LlamaStackConfigCmName)
1096+
if err != nil && !apierrors.IsNotFound(err) {
1097+
return nil, fmt.Errorf("failed to get Llama Stack ConfigMap resource version: %w", err)
1098+
}
1099+
mcpConfigMapResourceVersion, err := utils.GetConfigMapResourceVersion(r, ctx, utils.OpenShiftMCPServerConfigCmName)
1100+
if err != nil && !apierrors.IsNotFound(err) {
1101+
return nil, fmt.Errorf("failed to get MCP Server ConfigMap resource version: %w", err)
1102+
}
10651103

10661104
// Use helper functions to build common components
10671105
labels := buildCommonLabels()
@@ -1132,8 +1170,9 @@ func generateLCoreLibraryDeployment(r reconciler.Reconciler, ctx context.Context
11321170
Namespace: r.GetNamespace(),
11331171
Labels: labels,
11341172
Annotations: map[string]string{
1135-
utils.LCoreConfigMapResourceVersionAnnotation: lcoreConfigMapResourceVersion,
1136-
utils.LlamaStackConfigMapResourceVersionAnnotation: llamaStackConfigMapResourceVersion,
1173+
utils.LCoreConfigMapResourceVersionAnnotation: lcoreConfigMapResourceVersion,
1174+
utils.LlamaStackConfigMapResourceVersionAnnotation: llamaStackConfigMapResourceVersion,
1175+
utils.OpenShiftMCPServerConfigMapResourceVersionAnnotation: mcpConfigMapResourceVersion,
11371176
},
11381177
},
11391178
Spec: appsv1.DeploymentSpec{

0 commit comments

Comments
 (0)