fix(backup): do not get backing image custom resource #3372
Conversation
|
Warning Rate limit exceeded@mantissahz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces modifications to the backup target management system in Longhorn. The changes focus on enhancing the Changes
Sequence DiagramsequenceDiagram
participant Controller as BackupTargetController
participant Webhook as BackupBackingImageValidator
participant Store as BackupStore
Controller->>Store: Sync Backup Resources
Controller->>Controller: Add Sync Label
Controller->>Webhook: Validate Backup Backing Image
Webhook->>Webhook: Check Sync Label
Webhook->>Store: Verify Backing Image
Webhook-->>Controller: Validation Result
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
mantissahz
force-pushed
the
issue10026
branch
from
df1f721 to
c4b2597
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: 0
🧹 Nitpick comments (2)
webhook/resources/backupbackingimage/validator.go (1)
47-61: Validate both label presence and label content.Currently, the logic attempts to bypass the backing image existence check if a specific key is found in the Labels map. Consider verifying that the label does not just exist but also equals the expected value (though here it's set to an empty string). For instance, certain misconfigurations might occur if a different label key is used or if the user uses a label with a mismatched value.
types/types.go (1)
141-142: Add a quick note clarifying usage of 'BackupTargetSync'.The new constant clarifies that objects are brought in from a remote backup target. For maintainability, add a short clarifying comment next to this constant (or in your developer docs) indicating its usage patterns, especially that it is checked in admission webhooks to skip local references.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/backup_target_controller.go(1 hunks)types/types.go(1 hunks)webhook/resources/backupbackingimage/validator.go(1 hunks)
🔇 Additional comments (1)
controller/backup_target_controller.go (1)
663-665: Ensure backward compatibility when adding the new label.
By introducing the BackupTargetSync label, any existing logic that expects backups to always have a corresponding backing image resource in the cluster now has a condition to bypass it. Confirm that no external controllers or external scripts rely on the presence of backing images for all backup items. Otherwise, you may encounter unexpected issues where those components assume that the backing image always exists.
Would you like me to generate a script that searches for any external references to backing image existence checks?
| // The backup backing image might be created by the backup target controller synchronized from the remote backup target, | ||
| // and the backing image might not exist in the local cluster | ||
| exists := false | ||
| if backupBackingImage.Labels != nil { | ||
| _, exists = backupBackingImage.Labels[types.GetLonghornLabelKey(types.BackupTargetSync)] | ||
| } | ||
| // TODO: support backup for v2 data engine in the future | ||
| if types.IsDataEngineV2(backingImage.Spec.DataEngine) { | ||
| return werror.NewInvalidError(fmt.Sprintf("backing image %v uses v2 data engine which doesn't support backup operations", backingImageName), "") | ||
| if !exists { | ||
| backingImage, err := bbi.ds.GetBackingImageRO(backingImageName) | ||
| if err != nil { | ||
| return werror.NewInvalidError(fmt.Sprintf("failed to get the backing image %v for backup: %v", backingImageName, err), "") | ||
| } | ||
| // TODO: support backup for v2 data engine in the future | ||
| if types.IsDataEngineV2(backingImage.Spec.DataEngine) { | ||
| return werror.NewInvalidError(fmt.Sprintf("backing image %v uses v2 data engine which doesn't support backup operations", backingImageName), "") | ||
| } |
There was a problem hiding this comment.
I think we don't need to validate whether a backing image exists. We can allow users to create v2 backing image but always failed the backup (backupBacking.status.State=error, backupBackingImage.status.Error = "backing up v2 backing image is not supported). Then, we don't need the special label. WDYT?
cc @ChanYiLin
There was a problem hiding this comment.
Removed the label, and ignored the error ErrorIsNotFound. Thanks.
There was a problem hiding this comment.
After a second thought, I think using webhook is still more simple.
The current change in this PR is also good enough
If the backing image exists in the cluster and it is v2 backing image
then the webhook blocks the backup creation.
If the backing image doesn't exist in the cluster, then the backupbackingimage is pulled from the remote, we allow it.
There was a problem hiding this comment.
There are two different behaviors for v2 backup backingimage. Is making them consistent better?
if backup backing image is synchronized from remote backup target because backing image might not exist. ref longhorn/longhorn 10026, 6341 Signed-off-by: James Lu <[email protected]>
mantissahz
force-pushed
the
issue10026
branch
from
c4b2597 to
778870f
Compare
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue #ref longhorn/longhorn#10026, longhorn/longhorn#6341
What this PR does / why we need it:
Do not get the backing image custom resource if the backup backing image is synchronized from the remote backup target because the backing image might not exist.
Special notes for your reviewer:
Additional documentation or context