WIP: etcd backups - #2952
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
Hello @bhperry! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (16)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change replaces EtcdBackup PVC-only configuration with PVC or local storage, adds node and backup file status fields, and introduces typed job references. It adds the cluster-scoped EtcdBackupPolicy API with scheduling, node selection, storage, retention, and status fields. Matching CRD schemas and scheme registrations are updated, while config backup types are removed from config API registration and CRI-O credential provider types are registered. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
operator/v1alpha1/types_etcdbackuppolicy.go (1)
34-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo format validation for
Schedule(cron) orTimeZone, unlike the legacy Backup CRD it supersedes.The pre-existing
config.openshift.ioBackup.spec.etcd.schedule/timeZonefields enforced cron and IANA-timezone patterns; the newEtcdBackupPolicySpecfields have no equivalent, so invalid cron expressions can be persisted.
[recommended_refactor_low_effort_note]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/v1alpha1/types_etcdbackuppolicy.go` around lines 34 - 44, Add equivalent validation markers to EtcdBackupPolicySpec.Schedule and TimeZone, enforcing valid cron expressions for Schedule and valid IANA time-zone names for TimeZone as the superseded Backup CRD did. Preserve Schedule as required and TimeZone as optional while ensuring invalid values are rejected during CRD validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/v1alpha1/types_etcdbackup.go`:
- Around line 90-94: Update the kubebuilder XValidation message on the Path
field to say “path is immutable once set” instead of referring to “name”; leave
the immutability rule and other field metadata unchanged.
In `@operator/v1alpha1/types_etcdbackuppolicy.go`:
- Around line 52-55: Add non-negative validation for both NodeCount and
MaxQuantity using the appropriate Kubernetes validation markers, while
preserving NodeCount’s zero behavior for selecting all nodes. Mark NodeCount
with +optional consistently with its sibling fields, and update the
corresponding field declarations/documentation so generated CRD schemas reject
negative values.
- Around line 70-95: Update EtcdBackupPolicyRetentionRule to make the
discriminator enum accept both MaxQuantity and MaxSize. Add the corresponding
+unionMember tags to MaxQuantity and MaxSize, and add conditional XValidation
rules matching the EtcdBackupStorage union pattern so the field selected by Type
is required while the other is unset.
In `@payload-manifests/crds/0000_10_etcd_01_etcdbackups.crd.yaml`:
- Around line 88-94: Regenerate this CRD after correcting the kubebuilder
validation marker for the Path field in the EtcdBackup Go type, so the generated
validation message identifies “path” rather than “name.” Do not edit the
generated YAML directly.
---
Nitpick comments:
In `@operator/v1alpha1/types_etcdbackuppolicy.go`:
- Around line 34-44: Add equivalent validation markers to
EtcdBackupPolicySpec.Schedule and TimeZone, enforcing valid cron expressions for
Schedule and valid IANA time-zone names for TimeZone as the superseded Backup
CRD did. Preserve Schedule as required and TimeZone as optional while ensuring
invalid values are rejected during CRD validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b85840b1-9249-43dc-8ef7-89d067ce8593
⛔ Files ignored due to path filters (16)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1alpha1/zz_generated.featuregated-crd-manifests/backups.config.openshift.io/AutomatedEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1alpha1/zz_generated.model_name.gois excluded by!**/zz_generated*config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackuppolicies.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1alpha1/zz_generated.featuregated-crd-manifests/etcdbackuppolicies.operator.openshift.io/AutomatedEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1alpha1/zz_generated.featuregated-crd-manifests/etcdbackups.operator.openshift.io/AutomatedEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1alpha1/zz_generated.model_name.gois excluded by!**/zz_generated*operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (7)
config/v1alpha1/register.goconfig/v1alpha1/types_backup.gooperator/v1alpha1/register.gooperator/v1alpha1/types_etcdbackup.gooperator/v1alpha1/types_etcdbackuppolicy.gopayload-manifests/crds/0000_10_config-operator_01_backups.crd.yamlpayload-manifests/crds/0000_10_etcd_01_etcdbackups.crd.yaml
💤 Files with no reviewable changes (2)
- config/v1alpha1/types_backup.go
- config/v1alpha1/register.go
1 participant
No description provided.