[backups] Add indices to core backup resources #1719
Conversation
## What this PR does This adds custom indexable fields on core backup resources to enable filtering by backed-up application. This will later be useful for displaying backup resources in the dashboard: it will be possible to filter them with a field selector to display only those backup resources that are relevant to a given application. ### Release note ```release-note [backups] Enable filtering backup resources by backed up application for per-app views of backups. ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
WalkthroughThis pull request introduces Kubernetes API type definitions for backup management resources (Backup, BackupJob, Plan, RestoreJob) with corresponding scheme registration, deep copy support, and CRD schema definitions including selectable fields and status subresources for enhanced resource querying and lifecycle management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @lllamnyp, 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 introduces a significant improvement to how backup-related custom resources are managed and viewed. By adding indexable fields to Highlights
Using Gemini Code AssistThe 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
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 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. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds custom indexable fields to core backup resources, which is a great feature for improving filterability in the dashboard. The refactoring to move type registration into individual type files is a good architectural improvement, making the API definitions more modular.
I've found a couple of areas for potential improvement:
- The comments for exported variables in
groupversion_info.gowere removed. It would be good to restore them for better code documentation. - For consistency,
RestoreJobcould also be made filterable by its target application.
Additionally, it seems this PR also fixes a bug where the Plan.Status field was not being correctly deep-copied, which is an excellent correction. Overall, solid changes.
| GroupVersion = schema.GroupVersion{Group: "backups.cozystack.io", Version: "v1alpha1"} | ||
| SchemeBuilder = runtime.NewSchemeBuilder(addGroupVersion) | ||
| AddToScheme = SchemeBuilder.AddToScheme |
There was a problem hiding this comment.
The comments for these exported variables were removed. While the new layout is more compact, the comments were valuable for documenting the purpose of these variables. Consider re-adding them as line comments to maintain documentation without sacrificing the compact format.
| GroupVersion = schema.GroupVersion{Group: "backups.cozystack.io", Version: "v1alpha1"} | |
| SchemeBuilder = runtime.NewSchemeBuilder(addGroupVersion) | |
| AddToScheme = SchemeBuilder.AddToScheme | |
| GroupVersion = schema.GroupVersion{Group: "backups.cozystack.io", Version: "v1alpha1"} // group version used to register these objects | |
| SchemeBuilder = runtime.NewSchemeBuilder(addGroupVersion) // used to add go types to the GroupVersionKind scheme | |
| AddToScheme = SchemeBuilder.AddToScheme // adds the types in this group-version to the given scheme |
| // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.apiGroup` | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.kind` | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.name` |
There was a problem hiding this comment.
Adding these selectablefield annotations is a great improvement for filtering. For consistency across all backup-related resources, consider also adding similar annotations to the RestoreJob type. This would allow filtering RestoreJob resources by their target application via the .spec.targetApplicationRef field.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/backups/v1alpha1/backup_types.go (1)
95-100: Consider adding+kubebuilder:subresource:statusmarker.The
Backuptype has aStatusfield, but there's no+kubebuilder:subresource:statusmarker. Without this, status updates require full resource writes instead of using the/statussubresource endpoint. This is typically desired for types with a status to enable proper RBAC separation and optimistic concurrency on status updates.// The field indexing on applicationRef will be needed later to display per-app backup resources. // +kubebuilder:object:root=true +// +kubebuilder:subresource:status // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.apiGroup` // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.kind` // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.name`api/backups/v1alpha1/backupjob_types.go (1)
85-90: Consider adding+kubebuilder:subresource:statusmarker for consistency.Similar to
Backup,BackupJobhas aStatusfield but lacks the status subresource marker. Adding it enables proper status update semantics via the/statusendpoint.// The field indexing on applicationRef will be needed later to display per-app backup resources. // +kubebuilder:object:root=true +// +kubebuilder:subresource:status // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.apiGroup` // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.kind` // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.name`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/backups/v1alpha1/backup_types.go(2 hunks)api/backups/v1alpha1/backupjob_types.go(2 hunks)api/backups/v1alpha1/groupversion_info.go(1 hunks)api/backups/v1alpha1/plan_types.go(2 hunks)api/backups/v1alpha1/restorejob_types.go(1 hunks)api/backups/v1alpha1/zz_generated.deepcopy.go(2 hunks)packages/system/backup-controller/definitions/.gitattributes(1 hunks)packages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yaml(1 hunks)packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml(1 hunks)packages/system/backup-controller/definitions/backups.cozystack.io_plans.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
api/backups/v1alpha1/zz_generated.deepcopy.goapi/backups/v1alpha1/plan_types.goapi/backups/v1alpha1/backupjob_types.goapi/backups/v1alpha1/groupversion_info.goapi/backups/v1alpha1/backup_types.goapi/backups/v1alpha1/restorejob_types.go
**/zz_generated.*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated Go files matching pattern
zz_generated.*.go
Files:
api/backups/v1alpha1/zz_generated.deepcopy.go
🧬 Code graph analysis (6)
api/backups/v1alpha1/zz_generated.deepcopy.go (1)
api/backups/v1alpha1/plan_types.go (1)
PlanStatus(96-98)
api/backups/v1alpha1/plan_types.go (2)
api/backups/v1alpha1/groupversion_info.go (2)
SchemeBuilder(30-30)GroupVersion(29-29)pkg/apiserver/apiserver.go (1)
Scheme(53-53)
api/backups/v1alpha1/backupjob_types.go (2)
api/backups/v1alpha1/groupversion_info.go (2)
SchemeBuilder(30-30)GroupVersion(29-29)pkg/apiserver/apiserver.go (1)
Scheme(53-53)
api/backups/v1alpha1/groupversion_info.go (1)
pkg/apiserver/apiserver.go (1)
Scheme(53-53)
api/backups/v1alpha1/backup_types.go (2)
api/backups/v1alpha1/groupversion_info.go (2)
SchemeBuilder(30-30)GroupVersion(29-29)pkg/apiserver/apiserver.go (1)
Scheme(53-53)
api/backups/v1alpha1/restorejob_types.go (2)
api/backups/v1alpha1/groupversion_info.go (2)
SchemeBuilder(30-30)GroupVersion(29-29)pkg/apiserver/apiserver.go (1)
Scheme(53-53)
⏰ 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 (13)
packages/system/backup-controller/definitions/.gitattributes (1)
1-2: LGTM!Good practice to mark generated CRD files as
linguist-generatedwhile explicitly excluding.gitattributesitself so it remains visible in PRs and diffs.api/backups/v1alpha1/restorejob_types.go (1)
14-22: LGTM!The scheme registration follows the correct controller-runtime pattern, consistently registering both the type and its list variant. The absence of
selectableFieldsmarkers here is appropriate sinceRestoreJobreferences backups rather than applications directly.api/backups/v1alpha1/backup_types.go (1)
14-22: LGTM!Correct scheme registration pattern using
SchemeBuilder.RegisterwithAddKnownTypes.api/backups/v1alpha1/groupversion_info.go (1)
28-37: LGTM!The scheme registration follows the standard kubebuilder pattern:
addGroupVersionregisters the group version and standard metav1 types- Individual type files use
init()to register their specific types withSchemeBuilder.RegisterAddToSchemeaggregates all registrations for use by controllersapi/backups/v1alpha1/backupjob_types.go (1)
14-22: LGTM!Correct scheme registration pattern, consistent with other type files in this PR.
packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml (1)
233-236: No action needed. The optionalapiGroupfield in selectableFields is appropriate and aligns with Kubernetes standards. An empty or omittedapiGrouprepresents the core API group (/api/v1), which is valid Kubernetes semantics, not an edge case requiring special handling in field selector queries.Likely an incorrect or invalid review comment.
api/backups/v1alpha1/plan_types.go (2)
11-11: LGTM: Runtime import added for scheme registration.The import is necessary for the scheme registration logic added below.
14-22: LGTM: Scheme registration follows controller-runtime patterns.The init function correctly registers Plan and PlanList types with the SchemeBuilder using the standard AddKnownTypes pattern.
packages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yaml (1)
230-233: LGTM: selectableFields correctly configured for BackupJob.The selectableFields section correctly exposes the applicationRef subfields (apiGroup, kind, name) for field selector filtering, consistent with the PR objective to enable per-app filtering of backup resources.
api/backups/v1alpha1/zz_generated.deepcopy.go (1)
1-501: Auto-generated file - no review needed.This file is auto-generated by controller-gen (as indicated by the header comment and filename pattern). Per the coding guidelines for
**/zz_generated.*.gofiles, auto-generated files should not be edited or reviewed.packages/system/backup-controller/definitions/backups.cozystack.io_plans.yaml (3)
133-191: LGTM: Status schema correctly defined with standard Conditions.The status field schema follows Kubernetes conventions with a properly structured Conditions array, including all required fields and appropriate validation constraints.
193-196: LGTM: selectableFields correctly configured for Plan.The selectableFields section correctly exposes the applicationRef subfields for field selector filtering, consistent with the BackupJob CRD and the PR objective.
199-200: LGTM: Status subresource properly enabled.The status subresource configuration follows the standard Kubernetes pattern, allowing separate updates to the status field.
| // The field indexing on applicationRef will be needed later to display per-app backup resources. | ||
|
|
||
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:subresource:status | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.apiGroup` | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.kind` | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.applicationRef.name` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for field index setup for applicationRef fields in controller code
# Search for FieldIndexer or IndexField patterns
rg -nP -C5 'FieldIndexer|IndexField.*applicationRef' --type=go
# Also search for Plan reconciler setup
rg -nP -C5 'func.*SetupWithManager.*Plan' --type=goRepository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find Plan-related controller files
find . -type f -name "*.go" | grep -i plan | head -20
# List the directory structure around the provided file
ls -la api/backups/v1alpha1/ 2>/dev/null || echo "Directory not found"
# Search more broadly for SetupWithManager implementations
rg -n "SetupWithManager" --type=go | head -20
# Search for any field index setup patterns across the codebase
rg -n "IndexField\|SetupIndexes" --type=go | head -20Repository: cozystack/cozystack
Length of output: 3189
🏁 Script executed:
#!/bin/bash
# Read the PlanReconciler SetupWithManager method
sed -n '99,150p' internal/backupcontroller/plan_controller.go
# Also check the entire plan_controller.go file to look for any index setup
wc -l internal/backupcontroller/plan_controller.go
echo "---"
cat -n internal/backupcontroller/plan_controller.go | grep -i "index\|field"Repository: cozystack/cozystack
Length of output: 356
🏁 Script executed:
#!/bin/bash
# Read the plan_types.go file to see the full context around lines 36-42
cat -n api/backups/v1alpha1/plan_types.go | head -50Repository: cozystack/cozystack
Length of output: 1684
Add field indexes to the Plan controller to support efficient filtering by applicationRef subfields.
The kubebuilder selectablefield markers define JSONPath selectors for kubectl queries, but they require corresponding field indexes in the controller for efficient filtering. Add index setup in PlanReconciler.SetupWithManager() using mgr.GetFieldIndexer().IndexField() for .spec.applicationRef.apiGroup, .spec.applicationRef.kind, and .spec.applicationRef.name to support the "per-app backup resources" objective.
🤖 Prompt for AI Agents
In api/backups/v1alpha1/plan_types.go around lines 36-42 the kubebuilder
selectablefield markers were added for .spec.applicationRef.apiGroup,
.spec.applicationRef.kind and .spec.applicationRef.name but the Plan controller
lacks corresponding field indexes; update PlanReconciler.SetupWithManager() to
call mgr.GetFieldIndexer().IndexField(ctx, &Plan{},
"spec.applicationRef.apiGroup", func(obj client.Object) []string { ... }) (and
similarly for the two other paths) to extract and return the respective subfield
values, check and return any indexing errors, and ensure the index keys match
the JSONPath selectors so controller List/Watch filtering by those fields is
efficient.
## What this PR does This adds custom indexable fields on core backup resources to enable filtering by backed-up application. This will later be useful for displaying backup resources in the dashboard: it will be possible to filter them with a field selector to display only those backup resources that are relevant to a given application. ### Release note ```release-note [backups] Enable filtering backup resources by backed up application for per-app views of backups. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Introduced new Backup, BackupJob, Plan, and RestoreJob API resources for enhanced backup management. * Added selectable fields to backup resources, enabling efficient filtering and querying by application reference. * Added status subresource with conditions to Plan resources for improved status tracking and observability. * **Chores** * Updated repository metadata configuration. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 1ec14d6) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
…d backup system (#1867) ## What this PR does Update changelog for v1.0.0-alpha.1 to include missing features: - **Cozystack Operator**: New operator for Package and PackageSource management (#1740, #1741, #1755, #1756, #1760, #1761) - **Backup System**: Comprehensive backup functionality with Velero integration (#1640, #1685, #1687, #1708, #1719, #1720, #1737, #1762) - Add @androndo to contributors - Update Full Changelog link to v0.38.0...v1.0.0-alpha.1 ### Release note ```release-note [docs] Update changelog for v1.0.0-alpha.1: add cozystack-operator and backup system ```
What this PR does
This adds custom indexable fields on core backup resources to enable filtering by backed-up application. This will later be useful for displaying backup resources in the dashboard: it will be possible to filter them with a field selector to display only those backup resources that are relevant to a given application.
Release note
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.