controller add CozystackResourceDefinition reconciler #1313
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new Kubernetes controller, Changes
Sequence Diagram(s)sequenceDiagram
participant K8sAPI as Kubernetes API Server
participant Controller as CozystackResourceDefinitionReconciler
participant Deployment as cozystack-api Deployment
K8sAPI->>Controller: Event: CozystackResourceDefinition changed
Controller->>Controller: Update lastEvent timestamp
Controller->>Controller: Wait for debounce period
Controller->>K8sAPI: Get cozystack-api Deployment (cozy-system)
alt Deployment exists and event not handled
Controller->>K8sAPI: Patch pod template annotations (restartedAt)
K8sAPI->>Deployment: Trigger rolling restart
else Deployment not found or event already handled
Controller->>Controller: Log error or skip restart
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @klinch0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request adds a new Kubernetes controller to the cozystack-controller application. This controller is designed to watch for CozystackResourceDefinition custom resources and, upon detecting them, it will trigger a restart of the cozystack-api deployment. This mechanism likely ensures that any changes or configurations defined by CozystackResourceDefinition objects are applied by restarting the relevant API component.
Highlights
- New Reconciler: Introduces a new
CozystackResourceReconcilerresponsible for managingCozystackResourceDefinitioncustom resources. - Deployment Restart Logic: The
Reconcilemethod of the new controller fetches thecozystack-apiDeployment in thecozy-systemnamespace and updates itskubectl.kubernetes.io/restartedAtannotation. This action effectively triggers a rolling restart of thecozystack-apipods. - Controller Integration: The newly created
CozystackResourceReconcileris registered and set up with the main controller manager inmain.go, ensuring it starts and watches forCozystackResourceDefinitionobjects.
Using Gemini Code Assist
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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-code-assist comments or fill out our survey to provide feedback.
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.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces the CozystackResourceReconciler controller to restart the cozystack-api deployment upon changes to CozystackResourceDefinition resources. The review suggests making the target deployment configurable and using Patch to prevent race conditions.
| if deploy.Spec.Template.Annotations == nil { | ||
| deploy.Spec.Template.Annotations = map[string]string{} | ||
| } | ||
| deploy.Spec.Template.Annotations["kubectl.kubernetes.io/restartedAt"] = time.Now().Format(time.RFC3339) | ||
|
|
||
| if err := r.Update(ctx, deploy); err != nil { | ||
| log.Error(err, "cant update Deployment annotation") | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
The current Get-then-Update pattern can lead to race conditions if multiple CozystackResourceDefinition resources are modified concurrently. Using Patch instead of Update is a more robust approach to avoid optimistic locking conflicts.
| if deploy.Spec.Template.Annotations == nil { | |
| deploy.Spec.Template.Annotations = map[string]string{} | |
| } | |
| deploy.Spec.Template.Annotations["kubectl.kubernetes.io/restartedAt"] = time.Now().Format(time.RFC3339) | |
| if err := r.Update(ctx, deploy); err != nil { | |
| log.Error(err, "cant update Deployment annotation") | |
| return ctrl.Result{}, err | |
| } | |
| // Use a patch to avoid race conditions when multiple reconciles are triggered. | |
| patch := client.MergeFrom(deploy.DeepCopy()) | |
| if deploy.Spec.Template.Annotations == nil { | |
| deploy.Spec.Template.Annotations = make(map[string]string) | |
| } | |
| deploy.Spec.Template.Annotations["kubectl.kubernetes.io/restartedAt"] = time.Now().Format(time.RFC3339) | |
| if err := r.Patch(ctx, deploy, patch); err != nil { | |
| log.Error(err, "cannot patch Deployment annotation") | |
| return ctrl.Result{}, err | |
| } |
| Namespace: "cozy-system", | ||
| Name: "cozystack-api", |
There was a problem hiding this comment.
The namespace and name of the deployment to be restarted are hardcoded. Consider making these values configurable via command-line flags or environment variables to increase the controller's flexibility and reusability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/controller/cozystackresource_controller.go (1)
20-31: Consider making namespace and deployment name configurable.The hardcoded "cozy-system" namespace and "cozystack-api" deployment name reduce flexibility and make testing more difficult.
Consider adding these as configurable fields in the reconciler struct:
type CozystackResourceReconciler struct { client.Client Scheme *runtime.Scheme + DeploymentNamespace string + DeploymentName string }Then use them in the reconcile method:
err := r.Get(ctx, client.ObjectKey{ - Namespace: "cozy-system", - Name: "cozystack-api", + Namespace: r.DeploymentNamespace, + Name: r.DeploymentName, }, deploy)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cozystack-controller/main.go(1 hunks)internal/controller/cozystackresource_controller.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/clickhouse/README.md:60-67
Timestamp: 2025-07-03T05:54:51.264Z
Learning: The `cozy-lib.resources.sanitize` function in packages/library/cozy-lib/templates/_resources.tpl supports both standard Kubernetes resource format (with limits:/requests: sections) and flat format (direct resource specifications). The flat format takes priority over nested values. CozyStack apps include cozy-lib as a chart dependency through symlinks in packages/apps/*/charts/cozy-lib directories.
🧬 Code Graph Analysis (1)
internal/controller/cozystackresource_controller.go (2)
internal/controller/system_helm_reconciler.go (1)
Reconcile(34-81)internal/controller/tenant_helm_reconciler.go (1)
Reconcile(27-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (5)
internal/controller/cozystackresource_controller.go (4)
1-13: LGTM!The package declaration and imports are appropriate for a Kubernetes controller that watches custom resources and manages deployments.
15-18: LGTM!The struct definition follows the standard controller-runtime pattern with embedded client and scheme fields.
20-44: Verify if broad restart behavior is intentional.The controller restarts the
cozystack-apideployment for anyCozystackResourceDefinitionchange, regardless of which specific resource triggered the reconciliation (thereqparameter is unused).Is this broad restart behavior intentional? If the API server needs to restart for any CRD change, this is correct. Otherwise, consider using the
reqparameter to be more selective about when restarts occur.
47-51: LGTM!The SetupWithManager method follows the standard controller-runtime pattern for watching CozystackResourceDefinition resources.
cmd/cozystack-controller/main.go (1)
209-216: LGTM!The controller registration follows the established pattern used by other controllers in the same file. The error handling and logging are consistent and appropriate.
063622c to
6756d0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/controller/cozystackresource_controller.go (1)
25-26: Consider making deployment namespace and name configurable.The namespace and name of the deployment are hardcoded, which reduces flexibility and reusability of this controller.
🧹 Nitpick comments (1)
internal/controller/cozystackresource_controller.go (1)
47-51: Consider adding predicates to filter unnecessary reconciliation events.The controller will reconcile on every change to any CozystackResourceDefinition, including status updates. Consider adding predicates to filter events:
+import ( + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + func (r *CozystackResourceDefinitionReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&cozyv1alpha1.CozystackResourceDefinition{}). + WithEventFilter(predicate.GenerationChangedPredicate{}). Complete(r) }This will ensure the controller only reconciles when the spec changes, not on status updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cozystack-controller/main.go(1 hunks)internal/controller/cozystackresource_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cozystack-controller/main.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/clickhouse/README.md:60-67
Timestamp: 2025-07-03T05:54:51.264Z
Learning: The `cozy-lib.resources.sanitize` function in packages/library/cozy-lib/templates/_resources.tpl supports both standard Kubernetes resource format (with limits:/requests: sections) and flat format (direct resource specifications). The flat format takes priority over nested values. CozyStack apps include cozy-lib as a chart dependency through symlinks in packages/apps/*/charts/cozy-lib directories.
🧬 Code Graph Analysis (1)
internal/controller/cozystackresource_controller.go (2)
internal/controller/system_helm_reconciler.go (1)
Reconcile(34-81)internal/controller/tenant_helm_reconciler.go (1)
Reconcile(27-106)
| func (r *CozystackResourceDefinitionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||
| log := log.FromContext(ctx) | ||
|
|
||
| deploy := &appsv1.Deployment{} | ||
| err := r.Get(ctx, client.ObjectKey{ | ||
| Namespace: "cozy-system", | ||
| Name: "cozystack-api", | ||
| }, deploy) | ||
| if err != nil { | ||
| log.Error(err, "cant get Deployment cozy-system/cozystack-api") | ||
| return ctrl.Result{}, client.IgnoreNotFound(err) | ||
| } |
There was a problem hiding this comment.
Controller doesn't examine the CozystackResourceDefinition that triggered reconciliation.
The reconciler ignores the req parameter and doesn't fetch the actual CozystackResourceDefinition resource that triggered this reconciliation. This means:
- Every change to any CozystackResourceDefinition triggers the same action
- You cannot implement conditional logic based on the resource's spec or status
- Multiple rapid changes will cause multiple deployment restarts
Consider fetching and examining the CozystackResourceDefinition to implement more targeted restart logic.
func (r *CozystackResourceDefinitionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
log := log.FromContext(ctx)
+ // Fetch the CozystackResourceDefinition that triggered this reconciliation
+ crd := &cozyv1alpha1.CozystackResourceDefinition{}
+ if err := r.Get(ctx, req.NamespacedName, crd); err != nil {
+ return ctrl.Result{}, client.IgnoreNotFound(err)
+ }
+
+ // Add logic here to determine if restart is needed based on crd.Spec
+ // For example, you might only restart for certain types of changes
+
deploy := &appsv1.Deployment{}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *CozystackResourceDefinitionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | |
| log := log.FromContext(ctx) | |
| deploy := &appsv1.Deployment{} | |
| err := r.Get(ctx, client.ObjectKey{ | |
| Namespace: "cozy-system", | |
| Name: "cozystack-api", | |
| }, deploy) | |
| if err != nil { | |
| log.Error(err, "cant get Deployment cozy-system/cozystack-api") | |
| return ctrl.Result{}, client.IgnoreNotFound(err) | |
| } | |
| func (r *CozystackResourceDefinitionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | |
| log := log.FromContext(ctx) | |
| // Fetch the CozystackResourceDefinition that triggered this reconciliation | |
| crd := &cozyv1alpha1.CozystackResourceDefinition{} | |
| if err := r.Get(ctx, req.NamespacedName, crd); err != nil { | |
| return ctrl.Result{}, client.IgnoreNotFound(err) | |
| } | |
| // Add logic here to determine if restart is needed based on crd.Spec | |
| // For example, you might only restart for certain types of changes | |
| deploy := &appsv1.Deployment{} | |
| err := r.Get(ctx, client.ObjectKey{ | |
| Namespace: "cozy-system", | |
| Name: "cozystack-api", | |
| }, deploy) | |
| if err != nil { | |
| log.Error(err, "cant get Deployment cozy-system/cozystack-api") | |
| return ctrl.Result{}, client.IgnoreNotFound(err) | |
| } | |
| // … rest of reconcile logic … | |
| } |
🤖 Prompt for AI Agents
In internal/controller/cozystackresource_controller.go around lines 20 to 31,
the Reconcile function does not fetch or examine the CozystackResourceDefinition
resource that triggered the reconciliation, relying only on a fixed Deployment
fetch. Modify the code to first retrieve the CozystackResourceDefinition
instance using the req.NamespacedName, handle not-found errors appropriately,
and then use its spec or status to conditionally control the deployment restart
logic, enabling more precise and efficient reconciliations.
6756d0e to
f73aa39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/controller/cozystackresource_controller.go (1)
37-40: Hard-coded target Deployment & ignored CRD still unresolvedThe reconciler still short-circuits on
req.Namespace != "cozy-system" || req.Name != "cozystack-api", which means:
- The namespace / name are baked in, so the controller cannot be reused in other environments.
- The
reqrepresenting the CozystackResourceDefinition that triggered the event is still ignored, so you cannot apply conditional logic based on the CRD content.Both points were raised in previous reviews and remain unaddressed.
🧹 Nitpick comments (2)
internal/controller/cozystackresource_controller.go (2)
74-82: Add retry loop around the Patch to avoid transient conflictsEven with
MergeFrom, optimistic-locking conflicts can still occur if another controller mutates the Deployment simultaneously.
Wrap the patch call inretry.RetryOnConflict(fromk8s.io/client-go/util/retry) to make the restart more reliable under load.
100-104: Watching every Deployment is noisy
For(&appsv1.Deployment{})registers for all Deployment updates cluster-wide, yet the reconciler only cares about a single deployment.
Limit the watch withOwnsor use a predicate so unrelated Deployment churn doesn’t enqueue superfluous reconciliations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cozystack-controller/main.go(1 hunks)internal/controller/cozystackresource_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cozystack-controller/main.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| r.mu.Lock() | ||
| le := r.lastEvent | ||
| lh := r.lastHandled | ||
| debounce := r.Debounce | ||
| r.mu.Unlock() | ||
|
|
There was a problem hiding this comment.
Possible missed restarts due to TOCTOU on lastEvent / lastHandled
lastEvent is read under the lock (Lines 42-45) but lastHandled is written later (Lines 86-88) based on the old le value.
If another CRD event lands after you release the lock but before the patch completes, lastEvent is newer than lastHandled yet no new reconcile is queued, so the fresh event may never trigger a restart.
Patch either:
- r.lastHandled = le
+ r.lastHandled = time.Now()or keep the mutex held until lastHandled is updated.
Also applies to: 86-88
🤖 Prompt for AI Agents
In internal/controller/cozystackresource_controller.go around lines 42 to 47 and
86 to 88, there is a TOCTOU issue where lastEvent is read under the lock but
lastHandled is updated later outside the lock, risking missed restarts if new
events arrive in between. To fix this, either hold the mutex lock continuously
from reading lastEvent until after lastHandled is updated, or restructure the
code to update lastHandled while still holding the lock to ensure consistency
and prevent missing new events.
There was a problem hiding this comment.
Looks like valid behavior to me. r.lastHandled ends up being set to the time of the last event, so any subsequent CRD events have a later event time and are also handled, instead of being dropped for having an earlier time than r.lastHandled. This might cause more restarts, not less.
There was a problem hiding this comment.
@lllamnyp You’re right that the current sequence won’t drop a restart; the risk is extra restarts: if a new CRD event lands during the patch, writing lastHandled = le ( Lines 86–88) can trigger a second, redundant restart.
A minimal fix that preserves debounce and closes the race without holding the lock across I/O:
// after a successful patch of the Deployment
r.mu.Lock()
if le.After(r.lastHandled) {
r.lastHandled = time.Now() // cover all events up to completion time
}
r.mu.Unlock()This way, any events that arrived while patching are considered handled by the just-completed restart, avoiding unnecessary follow-up restarts, while still ensuring newer events after this point will be restarted.
Signed-off-by: kklinch0 <kklinch0@gmail.com>
f73aa39 to
8b1e55d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/controller/cozystackresource_controller.go (3)
37-41: Hard-coded target & CRD ignoredReconciler filters on a single hard-coded Deployment and never fetches the
CozystackResourceDefinitionthat triggered the event. This repeats the issue already raised in earlier reviews.
42-47: Race window betweenlastEventand restartReading the timestamps under the mutex and releasing it before the restart opens a gap where new events can arrive; combined with setting
lastHandled = lelater, this can schedule redundant restarts immediately after one just finished.Either keep the lock until
lastHandledis updated, or setlastHandled = time.Now()after the patch, as suggested previously.
86-88: UpdatelastHandledto actual completion timeSetting
lastHandled = lemeans events that occurred during the patch are considered “unhandled”, forcing an immediate second reconcile. Use the current time to cover everything up to the successful patch.- r.lastHandled = le + r.lastHandled = time.Now()
🧹 Nitpick comments (1)
internal/controller/cozystackresource_controller.go (1)
100-116: Make restart target configurableNamespace and Deployment name are embedded in three different places (
iffilter,Get, and EnqueueRequestsFromMapFunc`). Exposing these via flags or environment variables will make the controller reusable across environments and unit-testable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cozystack-controller/main.go(1 hunks)internal/controller/cozystackresource_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cozystack-controller/main.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| package controller | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing kubebuilder:rbac markers
The controller lacks RBAC comments required by controller-runtime scaffolding for generating the ClusterRole. Without them, make deploy (or equivalent) will install an empty role and the reconciler will fail with “forbidden” errors at runtime.
Add explicit RBAC lines for the CRD and for deployments patch/update permissions.
🤖 Prompt for AI Agents
In internal/controller/cozystackresource_controller.go at the top of the file,
add kubebuilder:rbac markers as comments to specify the required RBAC
permissions. Include lines granting access to the custom resource defined by the
CRD and also permissions to patch and update deployments. This will ensure that
when running `make deploy`, the generated ClusterRole includes the necessary
permissions to avoid forbidden errors during reconciliation.
What this PR does
Release note
Summary by CodeRabbit