diff --git a/kagenti-operator/internal/controller/agentruntime_controller.go b/kagenti-operator/internal/controller/agentruntime_controller.go index 814f68f5..18f69271 100644 --- a/kagenti-operator/internal/controller/agentruntime_controller.go +++ b/kagenti-operator/internal/controller/agentruntime_controller.go @@ -248,17 +248,9 @@ func (r *AgentRuntimeReconciler) Reconcile(ctx context.Context, req ctrl.Request // 6.5. Discover linked skills from workload annotation (set by kagenti backend or user) fg := r.getFeatureGates() + var linkedSkills []string if fg.SkillDiscovery { - rt.Status.LinkedSkills = r.readLinkedSkills(ctx, rt) - if len(rt.Status.LinkedSkills) > 0 { - r.setCondition(rt, ConditionTypeSkillsDiscovered, metav1.ConditionTrue, "SkillsFound", - fmt.Sprintf("%d linked skill(s) discovered from workload annotation", len(rt.Status.LinkedSkills))) - } else { - meta.RemoveStatusCondition(&rt.Status.Conditions, ConditionTypeSkillsDiscovered) - } - } else { - rt.Status.LinkedSkills = nil - meta.RemoveStatusCondition(&rt.Status.Conditions, ConditionTypeSkillsDiscovered) + linkedSkills = r.readLinkedSkills(ctx, rt) } // 7. Count configured pods @@ -267,12 +259,31 @@ func (r *AgentRuntimeReconciler) Reconcile(ctx context.Context, req ctrl.Request logger.V(1).Info("Failed to count configured pods", "error", err) } - // 8. Update status + // 8. Update status (retry on conflict to preserve all conditions computed above) rt.Status.ConfiguredPods = configuredPods r.setPhase(rt, agentv1alpha1.RuntimePhaseActive) r.setCondition(rt, ConditionTypeReady, metav1.ConditionTrue, "Configured", fmt.Sprintf("Workload %s configured with config-hash %s", rt.Spec.TargetRef.Name, configResult.Hash[:12])) - if err := r.Status().Update(ctx, rt); err != nil { + if fg.SkillDiscovery { + rt.Status.LinkedSkills = linkedSkills + if len(linkedSkills) > 0 { + r.setCondition(rt, ConditionTypeSkillsDiscovered, metav1.ConditionTrue, "SkillsFound", + fmt.Sprintf("%d linked skill(s) discovered from workload annotation", len(linkedSkills))) + } else { + meta.RemoveStatusCondition(&rt.Status.Conditions, ConditionTypeSkillsDiscovered) + } + } else { + rt.Status.LinkedSkills = nil + meta.RemoveStatusCondition(&rt.Status.Conditions, ConditionTypeSkillsDiscovered) + } + desired := rt.Status.DeepCopy() + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, req.NamespacedName, rt); err != nil { + return err + } + rt.Status = *desired // safe: this controller is the sole status owner + return r.Status().Update(ctx, rt) + }); err != nil { logger.Error(err, "Failed to update status") return ctrl.Result{}, err } @@ -708,10 +719,6 @@ func (r *AgentRuntimeReconciler) handleDeletion(ctx context.Context, rt *agentv1 delete(workloadLabels, LabelManagedBy) acc.obj.SetLabels(workloadLabels) - // Remove skills annotation from workload metadata. - workloadAnnotations := acc.obj.GetAnnotations() - delete(workloadAnnotations, AnnotationSkills) - acc.obj.SetAnnotations(workloadAnnotations) // Remove kagenti.io/type from PodTemplateSpec pod labels so future pods // are not presented to the webhook with the type label. diff --git a/kagenti-operator/internal/controller/agentruntime_controller_test.go b/kagenti-operator/internal/controller/agentruntime_controller_test.go index 69178b89..5d5fa179 100644 --- a/kagenti-operator/internal/controller/agentruntime_controller_test.go +++ b/kagenti-operator/internal/controller/agentruntime_controller_test.go @@ -209,6 +209,47 @@ var _ = Describe("AgentRuntime Controller", func() { Expect(k8sClient.Get(ctx, nn, updatedRT)).To(Succeed()) Expect(updatedRT.Status.LinkedSkills).To(ConsistOf("summarizer", "translator")) }) + + It("should persist linkedSkills even after a concurrent status modification", func() { + dep := newDeployment("skills-conflict-deploy", namespace) + dep.Annotations = map[string]string{ + AnnotationSkills: `["skill-a","skill-b"]`, + } + Expect(k8sClient.Create(ctx, dep)).To(Succeed()) + defer func() { _ = k8sClient.Delete(ctx, dep) }() + + rt := newAgentRuntime("skills-conflict-rt", namespace, "skills-conflict-deploy", agentv1alpha1.RuntimeTypeAgent) + Expect(k8sClient.Create(ctx, rt)).To(Succeed()) + defer func() { _ = k8sClient.Delete(ctx, rt) }() + + r := newReconciler() + r.GetFeatureGates = func() *webhookconfig.FeatureGates { + return &webhookconfig.FeatureGates{SkillDiscovery: true} + } + nn := types.NamespacedName{Name: "skills-conflict-rt", Namespace: namespace} + + // First reconcile to set up finalizer + _, _ = r.Reconcile(ctx, reconcile.Request{NamespacedName: nn}) + + // Simulate a concurrent modification by updating the object's status + // from another controller (this bumps resourceVersion) + current := &agentv1alpha1.AgentRuntime{} + Expect(k8sClient.Get(ctx, nn, current)).To(Succeed()) + current.Status.ConfiguredPods = 99 + Expect(k8sClient.Status().Update(ctx, current)).To(Succeed()) + + // Now reconcile — the status update inside Reconcile will initially + // hit a conflict (stale resourceVersion), but retry should succeed + _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: nn}) + Expect(err).NotTo(HaveOccurred()) + + updatedRT := &agentv1alpha1.AgentRuntime{} + Expect(k8sClient.Get(ctx, nn, updatedRT)).To(Succeed()) + Expect(updatedRT.Status.LinkedSkills).To(ConsistOf("skill-a", "skill-b")) + + configCond := meta.FindStatusCondition(updatedRT.Status.Conditions, ConditionTypeConfigResolved) + Expect(configCond).NotTo(BeNil(), "ConfigResolved condition must survive the retry re-fetch") + }) }) Context("When setting status", func() { diff --git a/kagenti-operator/test/e2e/e2e_test.go b/kagenti-operator/test/e2e/e2e_test.go index da60d992..c35c12ac 100644 --- a/kagenti-operator/test/e2e/e2e_test.go +++ b/kagenti-operator/test/e2e/e2e_test.go @@ -2133,6 +2133,13 @@ rules: Context("Feature gate enabled", Ordered, func() { BeforeAll(func() { + By("re-applying target Deployment to restore skills annotation after prior deletion cleanup") + _, err := utils.KubectlApplyStdin(skillDiscoveryDeploymentFixture(), skillDiscoveryTestNamespace) + Expect(err).NotTo(HaveOccurred()) + Expect(utils.WaitForDeploymentReady( + "skill-discovery-agent", skillDiscoveryTestNamespace, 2*time.Minute, + )).To(Succeed()) + By("enabling skillDiscovery feature gate") Expect(utils.EnableSkillDiscovery(controllerNamespace, controllerDeployment)).To(Succeed())