Repository navigation
br, pkg/objstore: add operation metadata to external storage locks - #69231
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note-none
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces operation-scoped lock metadata for BR external-storage locks. A new ChangesOperation-scoped lock metadata and BR integration
Sequence Diagram(s)sequenceDiagram
participant TaskEntry as RunStreamTruncate / RunStreamRestore
participant Config as task.Config
participant OpCtx as operation.Context
participant OpLock as operation.TryLockRemote / LockWithRetry*
participant MigExt as MigrationExt
participant ObjStore as objstore.TryLockRemote* / LockWithRetry
TaskEntry->>Config: EnsureOperationContext(cmdName)
Config->>OpCtx: Ensure(cmdName) → generates UUID OperationID, StartedAt
TaskEntry->>OpCtx: SetRestoreID(resolvedRestoreID)
TaskEntry->>OpLock: TryLockRemote(storage, path, opCtx, resource, hint)
OpLock->>OpCtx: LockMeta(resource, hint) → LockMetaInput
OpLock->>ObjStore: TryLockRemote(path, LockMetaInput)
ObjStore-->>OpLock: RemoteLock or ErrLocked{Path, Local, Blockers}
OpLock-->>TaskEntry: RemoteLock or error
TaskEntry->>MigExt: WithOperationContext(opCtx)
MigExt->>OpLock: LockWithRetryWrite(path, opCtx, resource, hint)
OpLock->>ObjStore: LockWithRetry(locker, path, LockMetaInput)
ObjStore-->>OpLock: RemoteLock or enriched ErrLocked
MigExt-->>TaskEntry: MergeAndMigratedTo, error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ti-chi-bot
Bot
added
the
size/XXL
|
Hi @RidRisR. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/objstore/locking.go`:
- Around line 151-160: The unbounded blocker hydration in the callback function
where blockers are appended via lockBlockerFromPath causes poor scaling under
heavy contention. Cap the number of blockers that are hydrated and parsed to a
fixed limit (e.g., first N), maintain a separate counter for the total blocker
count, and apply this cap at all locations where blockers are collected
(including the WalkDir callback). Modify the blocker-appending logic to stop
hydrating blockers after reaching the limit while continuing to count total
blockers, then serialize and log only the sampled blockers while reporting the
total blocker count separately in error messages and logs.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: e95fb8d4-b429-4129-82de-2c2973e150d3
📒 Files selected for processing (25)
CONTEXT.mdbr/pkg/operation/BUILD.bazelbr/pkg/operation/context.gobr/pkg/operation/context_test.gobr/pkg/operation/lock.gobr/pkg/restore/log_client/BUILD.bazelbr/pkg/restore/log_client/client.gobr/pkg/restore/log_client/client_test.gobr/pkg/restore/snap_client/BUILD.bazelbr/pkg/restore/snap_client/pitr_collector.gobr/pkg/restore/snap_client/pitr_collector_test.gobr/pkg/stream/BUILD.bazelbr/pkg/stream/stream_metas.gobr/pkg/stream/stream_metas_test.gobr/pkg/task/BUILD.bazelbr/pkg/task/common.gobr/pkg/task/operator/BUILD.bazelbr/pkg/task/operator/migrate_to.gobr/pkg/task/restore.gobr/pkg/task/stream.godocs/design/2026-06-15-br-external-storage-lock-owner.mdpkg/objstore/BUILD.bazelpkg/objstore/locking.gopkg/objstore/locking_test.gopkg/objstore/s3store/s3_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69231 +/- ##
================================================
- Coverage 76.3198% 75.8434% -0.4764%
================================================
Files 2041 2058 +17
Lines 561423 573303 +11880
================================================
+ Hits 428477 434813 +6336
- Misses 132043 137196 +5153
- Partials 903 1294 +391
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| Path string | ||
| Meta LockMeta | ||
| Local LockMetaInput | ||
| Blockers []LockBlocker |
There was a problem hiding this comment.
It seems blockers can replace Meta.
There was a problem hiding this comment.
I kept Meta intentionally as the primary/compatibility summary. Blockers is the newer detailed list, but Meta was the existing ErrLocked payload and is still useful when there is a single known lock or when older call paths enrich only one remote lock. When blockers are available, enrichErrLocked fills Meta from the first blocker, so consumers can use either the summary or the detailed list.
| if c.OperationID == "" { | ||
| return objstore.LockMetaInput{}, errors.New("operation ID is required") | ||
| } | ||
| if c.StartedAt.IsZero() { | ||
| return objstore.LockMetaInput{}, errors.New("operation started time is required") | ||
| } | ||
| if resource == "" { | ||
| return objstore.LockMetaInput{}, errors.New("lock resource type is required") | ||
| } |
There was a problem hiding this comment.
These fields can be lazily initialized when not found. resource can be an enum to avoid empty.
There was a problem hiding this comment.
Kept explicit validation here. I believe it is necessary for data integrity.
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
| type Context struct { | ||
| OperationID string `json:"operation_id"` |
There was a problem hiding this comment.
From the call sites in this PR, OperationID and StartedAt are initialized at the command boundary and then only read/copied into lock metadata. They do not appear to need later state transitions. Consider modeling these fields as a config/value snapshot fixed when Context is created, instead of allowing lower layers to lazily initialize the operation identity through Ensure. The only late-bound field here seems to be RestoreID, which can keep an explicit update path. This would make the ownership clearer: the command boundary allocates the operation identity, and storage locking only consumes already-initialized metadata.
| // LockMetaInput is the caller-provided metadata for a lock. | ||
| type LockMetaInput struct { | ||
| OwnerID string | ||
| LockType string |
There was a problem hiding this comment.
LockType may be easy to misread as the lock algorithm's read/write/exclusive mode, but from the current usage it is metadata describing the higher-level BR resource/scope, such as migration read/write/append or log-truncate-exclusive. Could we add a comment that explains this scope, and maybe use a more precise name such as ResourceType or LockResource? That would make it clearer that conflict behavior is still determined by the lock paths and read/write helpers, not by this metadata field.
| type Context struct { | ||
| OperationID string `json:"operation_id"` | ||
| StartedAt time.Time `json:"operation_started_at"` | ||
| RestoreID uint64 `json:"restore_id,omitempty"` |
There was a problem hiding this comment.
RestoreID makes this otherwise generic operation context depend on restore-specific lineage. That feels a bit surprising for a neutral package that is also used by stream migration/operator paths. Could we model this as neutral structured metadata instead, for example a small set of extra lock/log fields similar to []zap.Field, and let the restore layer add restore_id there? Then operation.Context would stay focused on command identity, while restore-specific metadata remains owned by the restore layer.
| if c.OperationID == "" { | ||
| return objstore.LockMetaInput{}, errors.New("operation ID is required") | ||
| } | ||
| if c.StartedAt.IsZero() { | ||
| return objstore.LockMetaInput{}, errors.New("operation started time is required") |
There was a problem hiding this comment.
Given they are assigned when creating, they are almost impossible to be zero.
| OperationID string `json:"operation_id"` | ||
| StartedAt time.Time `json:"operation_started_at"` | ||
| HintFields []HintField `json:"hint_fields,omitempty"` |
There was a problem hiding this comment.
Are we still need to serialize Context? If not, better to remove the json annotations.
Also HintFields can better be a map.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Leavrth, YuJuncen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
This reverts commit 13df4cb.
f728183 to
e9b7fff
Compare
|
In response to a cherrypick label: new pull request created to branch |
ti-chi-bot
Bot
removed
the
needs-cherry-pick-release-8.5
4 participants
What problem does this PR solve?
Issue Number: ref #67819
Problem Summary:
BR external-storage lock files are permanent until removed. When a BR process crashes or gets stuck, operators need enough metadata in the lock file and conflict logs to identify which BR execution created or is blocked by a lock.
What changed and how does it work?
This PR adds operation-aware metadata for BR-created external-storage locks:
pkg/objstore, includingoperation_id,operation_started_at,restore_id, andresource_type.br/pkg/operationto generate one command-scoped operation ID, log it at command start, attach restore ID when available, and build validated lock metadata.operator migrate-to.Check List
Tests
Manual test commands:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes