[cozystack-controller] Implement cache for CozystackResourceDefinitions - #1427

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

internal/shared/crdmem/memory.go (2)

68-88: Priming errors are silently ignored; add logs and success notice.

Lack of logging hides failures in cache priming and cache-sync issues.

Apply this diff:

 func (m *Memory) EnsurePrimingWithManager(mgr ctrl.Manager) error {
   var errOut error
   m.primeOnce.Do(func() {
     errOut = mgr.Add(runnable(func(ctx context.Context) error {
-      if ok := mgr.GetCache().WaitForCacheSync(ctx); !ok {
-        return nil
-      }
+      logger := ctrl.Log.WithName("crdmem")
+      if ok := mgr.GetCache().WaitForCacheSync(ctx); !ok {
+        logger.Error(nil, "cache sync failed; skipping priming")
+        return nil
+      }
       var list cozyv1alpha1.CozystackResourceDefinitionList
-      if err := mgr.GetClient().List(ctx, &list); err == nil {
-        for i := range list.Items {
-          m.Upsert(&list.Items[i])
-        }
-        m.mu.Lock()
-        m.primed = true
-        m.mu.Unlock()
-      }
+      if err := mgr.GetClient().List(ctx, &list); err != nil {
+        logger.Error(err, "failed to prime cache")
+        return nil
+      }
+      for i := range list.Items {
+        m.Upsert(&list.Items[i])
+      }
+      m.mu.Lock()
+      m.primed = true
+      m.mu.Unlock()
+      logger.Info("cache primed successfully", "count", len(list.Items))
       return nil
     }))
   })
   return errOut
 }

---

48-56: Snapshot must return deep copies to avoid cache corruption.

Shallow-copying structs that contain maps/slices shares backing storage with the cache; callers can mutate cached data.

Apply this diff:

 func (m *Memory) Snapshot() []cozyv1alpha1.CozystackResourceDefinition {
   m.mu.RLock()
   defer m.mu.RUnlock()
   out := make([]cozyv1alpha1.CozystackResourceDefinition, 0, len(m.data))
-  for _, v := range m.data {
-    out = append(out, v)
-  }
+  for _, v := range m.data {
+    out = append(out, *v.DeepCopy())
+  }
   return out
 }

internal/controller/cozystackresource_controller.go (2)

53-55: Duplicate lastEvent updates; keep it in the watch map-func only.

One source of truth avoids confusion.

Apply the deletions already shown in the previous diff (lines 53-55).

Also applies to: 86-88

---

46-67: Reconcile mixes CRD handling with Deployment-trigger requests; reorder to avoid false deletes.

When the mapped request for cozy-system/cozystack-api arrives, the code tries to Get a CRD named "cozystack-api" and may Delete it from memory on NotFound before calling debouncedRestart. Handle the special request first.

Apply this diff:

 func (r *CozystackResourceDefinitionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
   logger := log.FromContext(ctx)
 
+  // Handle the synthetic request that triggers the debounced restart first.
+  if req.Namespace == "cozy-system" && req.Name == "cozystack-api" {
+    return r.debouncedRestart(ctx, logger)
+  }
+
   crd := &cozyv1alpha1.CozystackResourceDefinition{}
   err := r.Get(ctx, types.NamespacedName{Name: req.Name}, crd)
   if err == nil {
     if r.mem != nil {
       r.mem.Upsert(crd)
     }
-    r.mu.Lock()
-    r.lastEvent = time.Now()
-    r.mu.Unlock()
     return ctrl.Result{}, nil
   }
-  if err != nil && !apierrors.IsNotFound(err) {
-    return ctrl.Result{}, err
-  }
-  if apierrors.IsNotFound(err) && r.mem != nil {
-    r.mem.Delete(req.Name)
-  }
-  if req.Namespace == "cozy-system" && req.Name == "cozystack-api" {
-    return r.debouncedRestart(ctx, logger)
-  }
-  return ctrl.Result{}, nil
+  if apierrors.IsNotFound(err) {
+    if r.mem != nil {
+      r.mem.Delete(req.Name)
+    }
+    return ctrl.Result{}, nil
+  }
+  return ctrl.Result{}, err
 }

internal/shared/crdmem/memory.go (1)

90-99: Optional: deep-copy API results for consistency.

ListFromCacheOrAPI returns live API objects by reference semantics; consider deep-copying for parity with Snapshot.

internal/controller/cozystackresource_controller.go (4)

126-151: Gate restarts until cache is primed to avoid hashing partial state.

Prevents early no-op patches and flapping before priming completes.

Apply this diff:

 func (r *CozystackResourceDefinitionReconciler) debouncedRestart(ctx context.Context, logger logr.Logger) (ctrl.Result, error) {
   r.mu.Lock()
   le := r.lastEvent
   lh := r.lastHandled
   debounce := r.Debounce
   r.mu.Unlock()
 
   if debounce <= 0 {
     debounce = 5 * time.Second
   }
   if le.IsZero() {
     return ctrl.Result{}, nil
   }
   if d := time.Since(le); d < debounce {
     return ctrl.Result{RequeueAfter: debounce - d}, nil
   }
   if !lh.Before(le) {
     return ctrl.Result{}, nil
   }
 
+  if r.mem == nil || !r.mem.IsPrimed() {
+    return ctrl.Result{RequeueAfter: 2 * time.Second}, nil
+  }
+
   newHash, err := r.computeConfigHash()
   if err != nil {
     return ctrl.Result{}, err
   }

---

161-167: Skip patch when hashes match, even if empty, to avoid useless API calls.

This avoids no-op patches when both sides are empty.

Apply this diff:

-  if oldHash == newHash && oldHash != "" {
+  if oldHash == newHash {
     r.mu.Lock()
     r.lastHandled = le
     r.mu.Unlock()
     logger.Info("No changes in CRD config; skipping restart", "hash", newHash)
     return ctrl.Result{}, nil
   }

---

82-83: Remove no-op WithPredicates() call.

Empty predicate list is redundant.

Apply this diff:

-    .For(&cozyv1alpha1.CozystackResourceDefinition{}, builder.WithPredicates()).
+    .For(&cozyv1alpha1.CozystackResourceDefinition{}).

---

156-172: Optional: extract annotation key into a constant.

Reduces typos and eases reuse.

Suggested snippet:

const configHashAnnotation = "cozystack.io/config-hash"

Then use configHashAnnotation at reads/writes.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro