Merge pull request #423 from pdettori/fix/reconciler-conflict-retry-422

Fix: wrap status update with RetryOnConflict to prevent linkedSkills loss

Author: pdettori

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.

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() {

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())