fix: failed to keep s3 bucket retain#595
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Pull request overview
This PR fixes S3 data being deleted when pvcRetentionPolicy=Delete is used together with s3RetentionPolicy=Retain, by ensuring released buckets do not run the S3 cleanup job during finalization. It also adjusts LogSet retention-policy getters to better handle older objects where only one of the two policies is set.
Changes:
- Skip S3 data deletion and directly remove the
BucketDataFinalizerwhen aBucketClaimis inStatusReleased(retain behavior). - Update
LogSetSpecretention-policy getters to implement PVC↔S3 inheritance when only one policy is present, defaulting toDeletewhen neither is set.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controllers/bucketclaim/controller.go | Avoids deleting S3 data when the bucket is already marked Released (retain semantics) by skipping the cleanup job and removing the finalizer. |
| api/core/v1alpha1/logset_types.go | Updates retention-policy getters to infer missing PVC/S3 policies for backward compatibility with older resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // if bucket is Released (s3RetentionPolicy=Retain), skip data deletion and release the finalizer directly | ||
| if bucket.Status.State == v1alpha1.StatusReleased { | ||
| ctx.Log.Info(fmt.Sprintf("skip data deletion for released bucket %v (s3RetentionPolicy=Retain)", client.ObjectKeyFromObject(ctx.Obj))) | ||
| controllerutil.RemoveFinalizer(bucket, v1alpha1.BucketDataFinalizer) | ||
| if err := ctx.Update(bucket); err != nil { |
| func (l *LogSetSpec) GetPVCRetentionPolicy() PVCRetentionPolicy { | ||
| if l.PVCRetentionPolicy == nil { | ||
| l.setDefaultRetentionPolicy() | ||
| if l.PVCRetentionPolicy != nil { | ||
| return *l.PVCRetentionPolicy | ||
| } | ||
| return *l.PVCRetentionPolicy | ||
| // inherit from s3 policy if only s3 is set (e.g. old objects without pvcRetentionPolicy) | ||
| if l.SharedStorage.S3 != nil && l.SharedStorage.S3.S3RetentionPolicy != nil { | ||
| return *l.SharedStorage.S3.S3RetentionPolicy | ||
| } | ||
| return PVCRetentionPolicyDelete |
What type of PR is this?
Which issue(s) this PR fixes:
Fixes # #594
What this PR does / why we need it:
Not Available
Special notes for your reviewer:
Not Available
Additional documentation (e.g. design docs, usage docs, etc.):
Not Available