DB Backup - Design doc and initial changes #1717
Conversation
dannyzaken
force-pushed
the
danny-db-backup
branch
from
1a886ad to
39140b4
Compare
WalkthroughAdds DB backup and recovery spec and corresponding status fields to the NooBaa CRD and public API types, generates deepcopy code for new types, updates the embedded CRD bundle checksum, and adds CNPG integration and DB backup/recovery design documentation plus a .gitignore entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant K8s as Kubernetes API
participant NooBaaCR as NooBaa CR
participant Operator as NooBaa Operator
participant CNPG as CloudNative-PG
participant VS as VolumeSnapshot
rect rgb(245,248,255)
note over User,Operator: Configure scheduled DB backups
User->>K8s: Apply NooBaa CR with spec.dbSpec.dbBackup
K8s-->>Operator: Reconcile NooBaa CR
Operator->>CNPG: Request snapshot (volumeSnapshotClass) per schedule
CNPG->>VS: Create VolumeSnapshot
VS-->>Operator: Snapshot created / metadata
Operator->>Operator: Prune snapshots to maxSnapshots
Operator->>K8s: Update status.dbStatus.backupStatus
end
alt Recovery requested
User->>K8s: Set spec.dbSpec.dbRecovery.volumeSnapshotName
K8s-->>Operator: Reconcile for recovery
Operator->>CNPG: Bootstrap DB cluster from VolumeSnapshot
CNPG-->>Operator: Recovery progress events
Operator->>K8s: Update status.dbStatus.recoveryStatus (Pending→Running→Completed/Failed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
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 |
dannyzaken
force-pushed
the
danny-db-backup
branch
from
39140b4 to
1cda15d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
deploy/crds/noobaa.io_noobaas.yaml(4 hunks)doc/design/noobaa-cnpg-db-backup.md(1 hunks)doc/noobaa-cnpg-integration.md(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(3 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(4 hunks)pkg/bundle/deploy.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (4)
DBBackupSpec(378-387)DBBackupStatus(557-569)DBRecoverySpec(390-393)DBRecoveryStatus(572-581)
⏰ 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). (12)
- GitHub Check: run-azure-vault-test
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-admission-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
dannyzaken
force-pushed
the
danny-db-backup
branch
2 times, most recently
from
1d633bc to
b8bdc36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
deploy/crds/noobaa.io_noobaas.yaml (1)
1212-1226: Add lower bound for backup retentionRight now
maxSnapshotsaccepts zero or negative values, which would make scheduled retention nonsensical and likely break reconciliation when pruning snapshots. Please add a schema guard (e.g.,minimum: 1or anx-kubernetes-validationsrule) so only positive counts are admitted.maxSnapshots: description: MaxSnapshots the maximum number of snapshots to keep. + minimum: 1 type: integerpkg/bundle/deploy.go (1)
2641-2655: Add server-side validation for the new backup knobsRight now the CRD accepts any integer/string for
maxSnapshotsandschedule. AmaxSnapshotsof0(or a negative value) or an empty/invalid cron expression will only be caught by the controller at runtime, leaving the object admitted and the reconcile loop stuck. We can prevent this class of misconfiguration by adding structural constraints (e.g.,minimum: 1formaxSnapshotsand a cron-pattern/x-kubernetes-validationscheck forschedule). Tightening the schema keeps invalid specs out of the API server and avoids noisy error loops.pkg/apis/noobaa/v1alpha1/noobaa_types.go (1)
556-581: Consider adding error fields to status types for better observability.The
DBBackupStatusandDBRecoveryStatustypes don't include error fields to report failures or issues. When backup/recovery operations fail, users would benefit from seeing error messages directly in the status.Consider extending the status types:
// DBBackupStatus reports the status of database backups type DBBackupStatus struct { // LastBackupTime timestamp of the last successful backup LastBackupTime *metav1.Time `json:"lastBackupTime,omitempty"` // NextBackupTime timestamp of the next scheduled backup NextBackupTime *metav1.Time `json:"nextBackupTime,omitempty"` // TotalSnapshots current number of snapshots TotalSnapshots int `json:"totalSnapshots,omitempty"` // AvailableSnapshots list of available snapshot names AvailableSnapshots []string `json:"availableSnapshots,omitempty"` + + // LastError contains the error message from the last failed backup operation + // +optional + LastError string `json:"lastError,omitempty"` } // DBRecoveryStatus reports the status of database recovery type DBRecoveryStatus struct { // Status current recovery status Status DBRecoveryStatusType `json:"status,omitempty"` // SnapshotName name of the snapshot being recovered from SnapshotName string `json:"snapshotName,omitempty"` // RecoveryTime timestamp when recovery was initiated RecoveryTime *metav1.Time `json:"recoveryTime,omitempty"` + + // ErrorMessage contains the error message if recovery failed + // +optional + ErrorMessage string `json:"errorMessage,omitempty"` }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(4 hunks)doc/design/noobaa-cnpg-db-backup.md(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(3 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(4 hunks)pkg/bundle/deploy.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (4)
DBBackupSpec(378-387)DBBackupStatus(557-569)DBRecoverySpec(390-393)DBRecoveryStatus(572-581)
⏰ 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). (13)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-admission-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-key-rotate-test
🔇 Additional comments (4)
.gitignore (1)
18-20: Good call on ignoring macOS cruftThe added pattern keeps stray Finder files out of the tree. 👍
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
1203-1511: DeepCopy coverage looks solidThe new DeepCopy helpers correctly cover backup/recovery spec and status, preventing pointer aliasing when reconciling CRs. Nicely done.
doc/design/noobaa-cnpg-db-backup.md (1)
1-397: LGTM! Design document is comprehensive and well-structured.The design document provides a thorough overview of the DB backup and recovery functionality. The code examples align with the API types defined in
pkg/apis/noobaa/v1alpha1/noobaa_types.go, and previously flagged critical issues have been addressed (ClassName field and VolumeSnapshot nested structure).pkg/apis/noobaa/v1alpha1/noobaa_types.go (1)
546-547: LGTM! Recovery status constant added appropriately.The
DBClusterStatusRecoveringconstant is properly added to theDBClusterStatustype to represent the recovery state, aligning well with the new recovery functionality.
dannyzaken
requested review from
a team,
jackyalbo,
karthikps97 and
nimrod-becker
and removed request for
a team and
karthikps97
dannyzaken
force-pushed
the
danny-db-backup
branch
from
b8bdc36 to
b3f4b20
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(4 hunks)doc/design/noobaa-cnpg-db-backup.md(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(3 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(5 hunks)pkg/bundle/deploy.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (5)
DBBackupSpec(378-388)VolumeSnapshotBackupSpec(390-400)DBBackupStatus(572-584)DBRecoverySpec(403-408)DBRecoveryStatus(587-596)
⏰ 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). (13)
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: golangci-lint
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-operator-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-hac-test
🔇 Additional comments (15)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (7)
367-375: LGTM!The DBBackup and DBRecovery fields are properly structured as optional pointers with appropriate JSON tags and descriptive comments.
377-388: Validation constraints properly address previous review feedback.The validation tags on
ScheduleandVolumeSnapshotfields properly enforce the constraints suggested in the previous review. The cron pattern validation and the Required tag ensure valid configurations.Based on past review comments.
390-400: Validation constraints properly address previous review feedback.The validation tags on
VolumeSnapshotClassandMaxSnapshotsfields ensure that invalid configurations cannot be set. This addresses the concerns raised in the previous review.Based on past review comments.
402-408: Validation constraint properly addresses previous review feedback.The
VolumeSnapshotNamefield has appropriate validation tags (Required and MinLength=1) as suggested in the previous review.Based on past review comments.
537-543: LGTM!The BackupStatus and RecoveryStatus fields are properly structured as optional status fields with appropriate comments.
561-562: LGTM!The
DBClusterStatusRecoveringconstant is properly defined and follows the existing pattern for database cluster status values.
571-616: LGTM!The status types are well-structured:
- DBBackupStatus properly tracks backup state with timestamps and snapshot list
- DBRecoveryStatus includes status enum, snapshot reference, and timestamp
- DBRecoveryStatusType enum covers all expected recovery states
- Time fields correctly use
*metav1.Timefor Kubernetes compatibilitypkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (8)
573-592: LGTM!The deepcopy methods for
DBBackupSpeccorrectly handle the nested pointer fieldVolumeSnapshotby creating a new instance and performing a deep copy.
594-621: LGTM!The deepcopy methods for
DBBackupStatuscorrectly handle:
- Time pointers using
DeepCopy()frommetav1.Time- String slice with proper allocation and copy
- Value-type fields
623-637: LGTM!The deepcopy methods for
DBRecoverySpecare correctly implemented for a struct containing only value-type fields.
639-657: LGTM!The deepcopy methods for
DBRecoveryStatuscorrectly handle theRecoveryTimepointer field usingDeepCopy()frommetav1.Time.
1241-1250: LGTM!The updated
NooBaaDBSpec.DeepCopyIntocorrectly handles the new fields:
DBBackupusesDeepCopyIntoto handle its nested pointer fieldDBRecoveryuses direct assignment since it only contains value-type fields
1267-1276: LGTM!The updated
NooBaaDBStatus.DeepCopyIntocorrectly handles the new status fields by callingDeepCopyIntoto properly deep copy their nested pointer fields.
1515-1515: LGTM!The change to call
DeepCopyIntoonDBStatusis correct and necessary, asNooBaaDBStatusnow contains nested pointer fields that require deep copying rather than shallow pointer assignment.
1745-1759: LGTM!The deepcopy methods for
VolumeSnapshotBackupSpecare correctly implemented for a struct containing only value-type fields.
dannyzaken
force-pushed
the
danny-db-backup
branch
from
b3f4b20 to
1ff9c5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
doc/design/noobaa-cnpg-db-backup.md (1)
48-73: Update YAML examples to match actual API field names.The examples still use
backupandrecovery, but the actual CRD and API types exposedbBackupanddbRecovery. Update the YAML snippets to use the correct field names.Apply this diff to fix the examples:
dbSpec: - backup: + dbBackup: - volumeSnapshotClass: "csi-snapshotter" # Required schedule: "0 0 1 * *" # Required: Cron format - maxSnapshots: 12 # Required: Retention count + volumeSnapshot: + volumeSnapshotClass: "csi-snapshotter" # Required + maxSnapshots: 12 # Required: Retention countAnd:
dbSpec: - recovery: + dbRecovery: volumeSnapshotName: "snapshot-jan-2025" # Required: Snapshot namepkg/bundle/deploy.go (1)
2643-2645: Cron schedule regex is incorrectly anchored and too permissive (wrap alternation and handle whitespace between fields).JSON‑schema
patternmatches substrings. With^(@...))|(((...))$, only the first alternative is anchored; strings ending in a valid token can pass. Also, the cron part doesn’t require whitespace separators between fields.Anchor the whole alternation and allow 5–7 whitespace‑separated fields:
- pattern: ^(@(annually|yearly|monthly|weekly|daily|hourly))|((((\d+,)+\d+|(\d+(\/|-)\d+)|\d+|\*) - ?){5,7})$ + pattern: ^(?:@(annually|yearly|monthly|weekly|daily|hourly)|(?:\s*[\d*/,-]+\s+){4,6}[\d*/,-]+)$This enforces either an @‑alias or 5–7 cron fields separated by spaces, and anchors the entire string.
🧹 Nitpick comments (6)
doc/design/noobaa-cnpg-db-backup.md (1)
200-221: Consider making online/offline backup configurable.The design hardcodes
Online: falsefor offline backups to ensure consistency, but this may not suit all use cases. Consider adding an optional field toDBBackupSpecto allow users to choose between online and offline backups based on their availability requirements.pkg/apis/noobaa/v1alpha1/noobaa_types.go (1)
402-408: Add optionalvolumeSnapshotNamespaceto DBRecoverySpec
ThevolumeSnapshotNamefield assumes the snapshot resides in the same namespace as the NooBaa CR; include an optional namespace field if cross-namespace recovery is required.pkg/bundle/deploy.go (4)
2635-2669: Spec: dbBackup looks good; consider clarifying cron semantics.
- Add a brief note on timezone and whether seconds/years (5–7 fields) are supported.
- If only volume snapshots are supported, consider
x-kubernetes-validationsto reject future mutually exclusive methods early.
2682-2693: Spec: dbRecovery minimal schema OK; consider name constraints.
- If
volumeSnapshotNamemust refer to a specific namespace/object, consider adding a pattern (DNS label) and documenting namespace resolution.
3461-3483: Status: backupStatus is fine; optional polish.
- If
availableSnapshotscan grow large, consider documenting/limiting reporting (e.g., newest N) to avoid status bloat.
3494-3507: Status: make recoveryStatus.status an enum.Define explicit states (e.g., Pending, InProgress, Succeeded, Failed) to aid clients and validation.
- status: - description: Status current recovery status - type: string + status: + description: Status current recovery status + type: string + enum: + - Pending + - InProgress + - Succeeded + - Failed
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(4 hunks)doc/design/noobaa-cnpg-db-backup.md(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(3 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(5 hunks)pkg/bundle/deploy.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (5)
DBBackupSpec(378-388)VolumeSnapshotBackupSpec(390-400)DBBackupStatus(572-584)DBRecoverySpec(403-408)DBRecoveryStatus(587-596)
⏰ 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). (13)
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-operator-tests
- GitHub Check: run-hac-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-admission-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-cli-tests
- GitHub Check: golangci-lint
🔇 Additional comments (14)
doc/design/noobaa-cnpg-db-backup.md (1)
297-320: Consider cleanup strategy for recovery configuration.The design doesn't specify how
dbSpec.dbRecoveryshould be handled after a successful recovery. Should users manually remove it, or should the operator automatically clear it once recovery completes? Consider documenting the expected behavior and lifecycle of the recovery configuration.pkg/apis/noobaa/v1alpha1/noobaa_types.go (3)
377-400: LGTM - Validation and structure are well-designed.The backup spec structure with validation tags is solid. The nested
VolumeSnapshotfield allows for future extensibility if additional backup methods are added, while the validation constraints ensure users provide valid configurations.
571-616: LGTM - Status types provide good observability.The status structures provide comprehensive information for monitoring backup and recovery operations. The inclusion of
AvailableSnapshotslist and detailed recovery status enum will enable useful CLI commands and dashboards.
561-562: LGTM - Consistent with existing status enum.The new
DBClusterStatusRecoveringconstant fits well with the existing status values and provides clear status reporting during recovery operations.deploy/crds/noobaa.io_noobaas.yaml (2)
1206-1263: LGTM - CRD schema correctly reflects API types.The OpenAPI schema for
dbBackupanddbRecoveryaccurately reflects the Go type definitions with proper validation rules and descriptions.
2031-2078: LGTM - Status schema accurately represents status types.The status schema correctly reflects the
DBBackupStatusandDBRecoveryStatustype definitions with appropriate field types and formats.pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (8)
573-592: LGTM!The DeepCopy methods for
DBBackupSpeccorrectly handle theVolumeSnapshotpointer field by creating a new instance and callingDeepCopyInto. The implementation follows the established patterns in this file.
594-621: LGTM!The DeepCopy methods for
DBBackupStatuscorrectly handle all pointer and slice fields:
LastBackupTimeandNextBackupTimeuseDeepCopy()for proper time value copyingAvailableSnapshotsslice is properly allocated and copiedThe implementation is correct and consistent with similar types in the file.
623-637: LGTM!The DeepCopy methods for
DBRecoverySpecuse direct struct copy, which is correct since the type only contains value fields (no pointers or slices). This is consistent with similar simple struct types in the file.
639-657: LGTM!The DeepCopy methods for
DBRecoveryStatuscorrectly handle theRecoveryTimepointer usingDeepCopy(), while value fields (StatusandSnapshotName) are copied directly. The implementation is correct.
1241-1250: LGTM!The modifications to
NooBaaDBSpec.DeepCopyIntocorrectly handle the new optional fields:
DBBackupusesDeepCopyIntobecause it contains nested pointer fieldsDBRecoveryuses direct assignment because it only contains value fieldsThis follows the established patterns in the file where the copy strategy is chosen based on the field's internal structure.
1267-1276: LGTM!The modifications to
NooBaaDBStatus.DeepCopyIntocorrectly deep copy bothBackupStatusandRecoveryStatususing their respectiveDeepCopyIntomethods. This is necessary because both status types contain pointer fields that require deep copying.
1515-1515: LGTM!The change to use
DeepCopyIntoforDBStatusis correct and necessary. SinceNooBaaDBStatusnow contains pointer fields (BackupStatusandRecoveryStatus), a deep copy method call is required instead of direct struct assignment.
1745-1759: LGTM!The DeepCopy methods for
VolumeSnapshotBackupSpecuse direct struct copy, which is correct since the type only contains value fields (VolumeSnapshotClassstring andMaxSnapshotsint). This is consistent with similar simple struct types throughout the file.
dannyzaken
force-pushed
the
danny-db-backup
branch
from
1ff9c5a to
ad3d34f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
deploy/crds/noobaa.io_noobaas.yaml (4)
1206-1237: Tighten validation for dbBackup (cron string, int format).Add minLength for schedule and int32 format for maxSnapshots for clearer API contracts.
dbBackup: @@ schedule: description: Schedule the schedule for the database backup in cron format. + minLength: 1 type: string @@ maxSnapshots: description: MaxSnapshots the maximum number of snapshots to keep. minimum: 1 + format: int32 type: integer
1251-1261: Validate volumeSnapshotName as a Kubernetes resource name.Enforce DNS-1123 pattern and length for VolumeSnapshot names.
volumeSnapshotName: description: VolumeSnapshotName specifies the name of the volume snapshot to recover from minLength: 1 + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string
2029-2051: Make availableSnapshots a set; mark totalSnapshots as int32.Avoid duplicates semantically and use a bounded integer format.
availableSnapshots: description: AvailableSnapshots list of available snapshot names items: type: string type: array + x-kubernetes-list-type: set @@ totalSnapshots: description: TotalSnapshots current number of snapshots + format: int32 type: integer
2063-2076: Constrain recoveryStatus fields (enum + non-empty snapshotName).Defines stable status values and prevents empty snapshot names.
snapshotName: description: SnapshotName name of the snapshot being recovered from + minLength: 1 type: string status: description: Status current recovery status - type: string + type: string + enum: + - Pending + - InProgress + - Succeeded + - Failedpkg/bundle/deploy.go (3)
2640-2644: Add a properly anchored cron pattern (restore validation, avoid the prior anchoring bug)
scheduleis now a plain string. That avoids the previous malformed regex, but removes validation. Please add a correctly anchored pattern that supports both @Aliases and 5/6‑field cron while anchoring the entire alternation.Apply this diff under
schedule::schedule: description: Schedule the schedule for the database backup in cron format. - type: string + type: string + # Accept @aliases or standard 5/6-field cron. Anchor the whole alternation. + # 5 fields (min hour dom mon dow) or 6 (sec min hour dom mon dow) + pattern: '^(?:(?:@(annually|yearly|monthly|weekly|daily|hourly))|(?:\d{1,2}|\*|(?:\d+[-/]\d+)|(?:\d+(?:,\d+)+))(\s+(?:\d{1,2}|\*|(?:\d+[-/]\d+)|(?:\d+(?:,\d+)+))){4,5})$'This restores strong validation and avoids only anchoring the first alternative.
3461-3467: Treat availableSnapshots as a set and bound totalSnapshotsMake the schema reflect intent:
- availableSnapshots should be unique names → model as a set.
- totalSnapshots shouldn’t be negative.
availableSnapshots: description: AvailableSnapshots list of available snapshot names items: type: string - type: array + type: array + x-kubernetes-list-type: set @@ totalSnapshots: description: TotalSnapshots current number of snapshots - type: integer + type: integer + minimum: 0
3503-3505: Constrain recoveryStatus.status with an enumMake status machine-readable and predictable.
status: description: Status current recovery status - type: string + type: string + enum: + - Pending + - InProgress + - Succeeded + - Failed + - Unknown
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(4 hunks)doc/design/noobaa-cnpg-db-backup.md(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(3 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(5 hunks)pkg/bundle/deploy.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- doc/design/noobaa-cnpg-db-backup.md
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (5)
DBBackupSpec(378-387)VolumeSnapshotBackupSpec(389-399)DBBackupStatus(571-583)DBRecoverySpec(402-407)DBRecoveryStatus(586-595)
⏰ 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). (13)
- GitHub Check: run-kms-dev-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-admission-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-cli-tests
🔇 Additional comments (3)
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
573-1759: LGTM! Auto-generated deepcopy code is correct.The generated deepcopy methods properly handle all pointer types, slices, and nested structures for the new backup and recovery types:
- Correctly uses
DeepCopy()formetav1.Timepointers- Properly allocates and copies nested structs (
DBBackupSpec,DBRecoverySpec, etc.)- The change from direct assignment to
DeepCopyIntocall forDBStatusinNooBaaStatus(line 1515) is the correct approach for proper deep copyingpkg/apis/noobaa/v1alpha1/noobaa_types.go (2)
367-407: LGTM! DB backup and recovery spec definitions are well-designed.The type definitions include proper validation constraints as requested in previous review:
- All required fields have
+kubebuilder:validation:Requiredtags- String fields have
MinLength=1validation to prevent empty valuesMaxSnapshotshasMinimum=1validation- The typo "Currntly" has been corrected to "Currently" (line 384)
The structure is clean and follows Kubernetes API conventions.
536-615: LGTM! DB backup and recovery status types are well-structured.The status types provide comprehensive tracking for backup and recovery operations:
DBBackupStatusincludes timestamps, snapshot counts, and available snapshot namesDBRecoveryStatustracks recovery state with appropriate enum values- New
DBClusterStatusRecoveringconstant properly extends the cluster status enumDBRecoveryStatusTypeenum covers all expected recovery states (None, Pending, Running, Completed, Failed)All fields use appropriate types and are marked as optional for status reporting.
dannyzaken
force-pushed
the
danny-db-backup
branch
from
ad3d34f to
1b56508
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (1)
377-387: Verify if pattern validation for Schedule was intentionally omitted.The
Schedulefield hasRequiredvalidation but lacks the cron pattern validation that was suggested in a previous review and marked as addressed. Current code only validates presence, not format.Run the following script to check the git history for the pattern validation:
#!/bin/bash # Check if pattern validation for Schedule field was ever added and possibly removed git log -p --all -S 'kubebuilder:validation:Pattern' -- pkg/apis/noobaa/v1alpha1/noobaa_types.go | head -100
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(4 hunks)doc/design/noobaa-cnpg-db-backup.md(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(3 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(5 hunks)pkg/bundle/deploy.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- doc/design/noobaa-cnpg-db-backup.md
- deploy/crds/noobaa.io_noobaas.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (1)
pkg/apis/noobaa/v1alpha1/noobaa_types.go (5)
DBBackupSpec(378-387)VolumeSnapshotBackupSpec(389-399)DBBackupStatus(571-583)DBRecoverySpec(402-407)DBRecoveryStatus(586-595)
⏰ 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). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-hac-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-kmip-test
🔇 Additional comments (13)
.gitignore (1)
18-19: Nice hygiene improvement!Thanks for adding the comment and
.DS_Storeignore—all good on my end.pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go (5)
573-657: LGTM! Auto-generated DeepCopy methods are correct.The newly generated DeepCopy methods for
DBBackupSpec,DBBackupStatus,DBRecoverySpec, andDBRecoveryStatusproperly handle:
- Pointer fields using
DeepCopy()formetav1.Timetypes- Nested struct pointers with
DeepCopyInto()- Slice fields with proper allocation and copying
1241-1250: LGTM! Correct deep-copy handling for new backup/recovery fields.The
NooBaaDBSpec.DeepCopyIntocorrectly handles the new optional fields:
DBBackupusesDeepCopyIntosinceDBBackupSpeccontains a pointer fieldDBRecoveryuses shallow copy (**out = **in) sinceDBRecoverySpeconly contains a string field
1267-1276: LGTM! Correct deep-copy handling for status fields.The
NooBaaDBStatus.DeepCopyIntocorrectly handles the new optional status fields usingDeepCopyIntoto ensure proper deep copying of nested pointer fields.
1515-1515: LGTM! Correct update to use DeepCopyInto.Changed from direct assignment to
DeepCopyIntocall, which is necessary sinceNooBaaDBStatusnow contains pointer fields that require deep copying.
1745-1759: LGTM! VolumeSnapshotBackupSpec DeepCopy is correct.Simple shallow copy is appropriate since the struct contains only primitive types (string and int).
pkg/apis/noobaa/v1alpha1/noobaa_types.go (7)
367-374: LGTM! Optional backup and recovery configuration fields.The new
DBBackupandDBRecoveryfields are properly documented and marked as optional, allowing users to configure database backup and recovery as needed.
389-399: LGTM! Proper validation for volume snapshot configuration.The
VolumeSnapshotBackupSpechas appropriate validation:
VolumeSnapshotClassrequires non-empty stringMaxSnapshotsrequires positive integer
401-407: LGTM! Proper validation for recovery specification.The
DBRecoverySpeccorrectly requires a non-empty volume snapshot name.
536-542: LGTM! Status fields for backup and recovery tracking.The optional
BackupStatusandRecoveryStatusfields enable proper observability of backup/recovery operations.
560-561: LGTM! New cluster status constant.The
DBClusterStatusRecoveringconstant properly extends the existing status enum for recovery scenarios.
570-583: LGTM! Well-structured backup status type.The
DBBackupStatusprovides comprehensive telemetry:
- Timestamps for last and next backup
- Snapshot count and list
All fields appropriately optional for status reporting.
585-615: LGTM! Complete recovery status tracking.The
DBRecoveryStatusandDBRecoveryStatusTypeprovide clear state machine for recovery:
- Five well-defined states (None, Pending, Running, Completed, Failed)
- Captures snapshot name and recovery timestamp
- Enables proper status reporting
| schedule: | ||
| description: Schedule the schedule for the database backup | ||
| in cron format. | ||
| type: string | ||
| volumeSnapshot: | ||
| description: |- | ||
| VolumeSnapshot the volume snapshot backup configuration. | ||
| Currently this is the only supported backup method and hence it is required. | ||
| properties: | ||
| maxSnapshots: | ||
| description: MaxSnapshots the maximum number of snapshots | ||
| to keep. | ||
| minimum: 1 | ||
| type: integer | ||
| volumeSnapshotClass: | ||
| description: VolumeSnapshotClass the volume snapshot class | ||
| for the database volume. | ||
| minLength: 1 | ||
| type: string | ||
| required: | ||
| - maxSnapshots | ||
| - volumeSnapshotClass | ||
| type: object | ||
| required: | ||
| - schedule | ||
| - volumeSnapshot | ||
| type: object |
There was a problem hiding this comment.
Add a basic non-empty validation for dbBackup.schedule.
dbBackup.schedule is required but the schema lets an empty string through. The backup controller will choke on an empty cron expression, so we should at least enforce minLength: 1 (ideally reuse the existing cron regex used elsewhere in the CRD). Please add minLength: 1 to this field (or the shared cron pattern) so bogus values get rejected at admission.
🤖 Prompt for AI Agents
In pkg/bundle/deploy.go around lines 2640 to 2666, the dbBackup.schedule field
is required but allows an empty string; update the CRD schema to add validation
by adding minLength: 1 to the schedule property (or alternatively apply the
existing cron pattern used elsewhere in the CRD) so empty cron expressions are
rejected at admission; ensure the added validation follows the same YAML
indentation and style as the surrounding properties.
Signed-off-by: Danny Zaken <[email protected]> doc changes
Signed-off-by: Danny Zaken <[email protected]>
dannyzaken
force-pushed
the
danny-db-backup
branch
from
1b56508 to
638e528
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/bundle/deploy.go (1)
2645-2647: Cron validation fordbSpec.dbBackup.schedule: add anchored pattern andminLength: 1.Currently it’s a plain string. Please enforce a fully‑anchored cron (including @Aliases) and require non‑empty values.
Apply this diff under the
schedule:property:- schedule: - description: Schedule the schedule for the database backup - in cron format. - type: string + schedule: + description: Schedule for the database backup in cron format (e.g. "0 2 * * *" or "@daily"). + type: string + minLength: 1 + # Either an @alias or a 5‑field cron. Entire string must match. + pattern: '^(?:(?:@(annually|yearly|monthly|weekly|daily|hourly)))|(?:^([0-5]?\d)\s+([01]?\d|2[0-3])\s+(\*|[1-9]|[12]\d|3[01])\s+(\*|[1-9]|1[0-2])\s+(\*|[0-6])$)$'
🧹 Nitpick comments (5)
pkg/bundle/deploy.go (4)
3472-3477: Array semantics forbackupStatus.availableSnapshots.Declare list type to avoid merge/patch surprises and improve OpenAPI clarity.
availableSnapshots: description: AvailableSnapshots list of available snapshot names items: type: string - type: array + type: array + x-kubernetes-list-type: atomic
3488-3490: Non‑negative constraint forbackupStatus.totalSnapshots.Add a lower bound.
totalSnapshots: description: TotalSnapshots current number of snapshots - type: integer + type: integer + minimum: 0
3513-3515: StabilizerecoveryStatus.statuswith an enum.Use a small set of states to improve UX and tooling (e.g., Pending, InProgress, Succeeded, Failed).
status: description: Status current recovery status - type: string + type: string + enum: + - Pending + - InProgress + - Succeeded + - Failed
2645-2647: Tiny doc nit: simplify wording and add examples.“Schedule the schedule…” is awkward. Consider concise text and examples.
- description: Schedule the schedule for the database backup - in cron format. + description: Schedule for the database backup in cron format (e.g. "0 2 * * *" or "@daily").deploy/crds/noobaa.io_noobaas.yaml (1)
1210-1241: Minor: Clarify theschedulefield description.Line 1216 has awkward phrasing: "Schedule the schedule for the database backup..." Consider rewording to: "Schedule for the database backup in cron format."
Additionally, verify if validation patterns are needed for the
schedulefield. Many CRDs constrain cron expressions with a regex pattern or delegate validation to the controller. Check if this is expected behavior here or if a pattern constraint should be added.schedule: - description: Schedule the schedule for the database backup + description: Schedule for the database backup in cron format. type: stringAre cron schedule format validations expected at the schema level (via regex pattern) or handled by the controller logic?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)deploy/crds/noobaa.io_noobaas.yaml(4 hunks)doc/design/noobaa-cnpg-db-backup.md(1 hunks)doc/noobaa-cnpg-integration.md(1 hunks)pkg/apis/noobaa/v1alpha1/noobaa_types.go(3 hunks)pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go(5 hunks)pkg/bundle/deploy.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- pkg/apis/noobaa/v1alpha1/zz_generated.deepcopy.go
- doc/design/noobaa-cnpg-db-backup.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-14T10:59:21.959Z
Learnt from: Neon-White
Repo: noobaa/noobaa-operator PR: 1587
File: pkg/system/reconciler.go:287-291
Timestamp: 2025-07-14T10:59:21.959Z
Learning: NooBaa operator design constraint: Multiple NooBaa instances are not supported in the same namespace, only across different namespaces. This means hard-coded resource names like ConfigMaps are acceptable within a namespace scope.
Applied to files:
doc/noobaa-cnpg-integration.mddeploy/crds/noobaa.io_noobaas.yaml
🪛 LanguageTool
doc/noobaa-cnpg-integration.md
[grammar] ~7-~7: Use a hyphen to join words.
Context: ...ps://cloudnative-pg.io/) to provide high availability PostgreSQL database deploym...
(QB_NEW_EN_HYPHEN)
⏰ 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). (13)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-operator-tests
- GitHub Check: run-cli-tests
- GitHub Check: run-admission-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-hac-test
🔇 Additional comments (9)
doc/noobaa-cnpg-integration.md (1)
1-250: Excellent comprehensive documentation for CNPG integration!This documentation provides thorough coverage of the NooBaa CloudNative-PG integration, including:
- Clear introduction to benefits and concepts
- Detailed CLI integration guide
- Operator reconciliation flow
- Low-level code architecture
- Configuration examples and best practices
- Security considerations
- Practical troubleshooting guidance
The structure is logical and serves as a valuable reference for both developers implementing the integration and operators deploying NooBaa with CNPG.
pkg/apis/noobaa/v1alpha1/noobaa_types.go (6)
371-380: Well-designed API additions for backup and recovery configuration.The new
DBBackupandDBRecoveryfields follow Kubernetes API conventions:
- Appropriate use of pointer types for optional fields
- Clear documentation comments
- Consistent JSON naming and tags
382-404: Well-structured backup specification types with proper validation.The
DBBackupSpecandVolumeSnapshotBackupSpectypes are well-designed:
- Required fields have appropriate validation tags (
Required,MinLength=1,Minimum=1)- Clear documentation explaining the backup method
- The use of pointer type for
VolumeSnapshotwithomitemptyandRequiredfollows standard Kubernetes API patterns (omitempty controls JSON marshaling while Required controls CRD validation)
406-412: Clean and focused recovery specification.The
DBRecoverySpectype is simple and well-validated with appropriateRequiredandMinLengthconstraints.
540-548: Appropriate status field additions.The new
BackupStatusandRecoveryStatusfields follow Kubernetes status conventions with optional pointer types and clear documentation.
565-567: Consistent status constant addition.The
DBClusterStatusRecoveringconstant follows the established naming pattern and logically extends the cluster status lifecycle to include recovery operations.
575-620: Comprehensive and well-designed status types.The status types are well-structured with appropriate fields and types:
- DBBackupStatus: Provides complete backup state with timestamps, counts, and snapshot list
- DBRecoveryStatus: Clear recovery state tracking with typed status field
- DBRecoveryStatusType: Well-defined state machine covering the complete recovery lifecycle (None → Pending → Running → Completed/Failed)
The use of pointer types for optional timestamp fields and typed enums for status values follows Kubernetes API best practices.
deploy/crds/noobaa.io_noobaas.yaml (2)
2072-2086: Verify recovery status field constraints.The
recoveryStatus.statusfield (line 2083-2085) currently has no enum or pattern constraints. Verify whether valid values (e.g., "in-progress", "completed", "failed") should be defined in the schema or if the controller documents these values elsewhere.Should
recoveryStatus.statusinclude an enum constraint listing valid recovery states? If so, what are the expected values?
1207-1343: Verify mutual exclusivity ofdbBackupanddbRecovery.Both
dbBackupanddbRecoveryare optional fields underspec.dbSpec, but setting both simultaneously may not be sensible (you typically backup or recover, not both). Verify whether the controller enforces mutual exclusivity, or if the schema should document this constraint via a CEL validation rule or annotation.Is mutual exclusivity between
dbBackupanddbRecoveryenforced by the controller, or should it be defined in the CRD schema (e.g., using validation rules)?
2 participants
Explain the changes
noobaa-cnpg-integration.mddoc.noobaa-cnpg-db-backup.mdNooBaaDBSpecandNooBaaDBStatustypes to support backup and recovery.Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Documentation
Chores