br: add keyspace-aware GC safepoint support for backup and restore (#65483) - #68863
ti-chi-bot wants to merge 4 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@RidRisR This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR moves BR GC safe-point handling from ChangesGC Manager Abstraction and Keyspace Support
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
br/pkg/gc/safepoint.go (1)
78-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject or clamp TTLs below 3 seconds before creating the ticker.
updateGapTimeis computed with integer division, sosp.TTL == 1or2makes Line 91 evaluate to0, andtime.NewTicker(0)panics. Since GC TTL is user-configurable, this is a reachable crash path.Proposed fix
if sp.ID == "" || sp.TTL <= 0 { return errors.Annotatef(berrors.ErrInvalidArgument, "invalid service safe point %v", sp) } + if sp.TTL < preUpdateServiceSafePointFactor { + return errors.Annotatef( + berrors.ErrInvalidArgument, + "service safe point TTL %d is too small; must be at least %d seconds", + sp.TTL, + preUpdateServiceSafePointFactor, + ) + } if err := CheckGCSafePoint(ctx, mgr, sp.BackupTS); err != nil { return errors.Trace(err) } @@ - updateGapTime := time.Duration(sp.TTL) * time.Second / preUpdateServiceSafePointFactor + updateGapTime := time.Duration(sp.TTL) * time.Second / preUpdateServiceSafePointFactor updateTick := time.NewTicker(updateGapTime)🤖 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 `@br/pkg/gc/safepoint.go` around lines 78 - 93, The code can panic when sp.TTL is 1 or 2 because updateGapTime becomes zero and time.NewTicker(0) panics; before creating the ticker (and before calling mgr.SetServiceSafePoint), validate or clamp sp.TTL to a safe minimum (e.g., reject TTLs < 3 seconds with an error from the same validation block that checks sp.ID/TTL, or set sp.TTL = 3 when computing updateGapTime), then compute updateGapTime = time.Duration(sp.TTL) * time.Second / preUpdateServiceSafePointFactor and create updateTick and checkTick; reference sp.TTL, updateGapTime, preUpdateServiceSafePointFactor, updateTick and the earlier validation that returns ErrInvalidArgument so the fix is applied in the same validation/initialization area.
🧹 Nitpick comments (3)
br/pkg/conn/conn.go (1)
279-285: ⚡ Quick winAdd a doc comment for
GetGCManager.
SetGcManageris documented, but the new exported getter is not. Please add a short doc comment so the exported pair stays consistent and godoc-clean.As per coding guidelines, "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement."
🤖 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 `@br/pkg/conn/conn.go` around lines 279 - 285, Add a godoc comment for the exported GetGCManager method on type Mgr to match the existing SetGcManager documentation and keep exported-symbol docs consistent; write a concise, semantic comment above func (mgr *Mgr) GetGCManager() gc.Manager that explains what GC manager is returned or under what conditions (e.g., "GetGCManager returns the current garbage-collection manager used by the Mgr." or similar) without restating the method name.br/tests/br_gc_keyspace_full_backup/run.sh (1)
74-81: ⚡ Quick winAlso assert the keyspace delete marker stays absent in global mode.
This script only proves
SIG_KS_SETwas not written. A regression that routes cleanup throughDeleteGCBarrierwould still pass, becauseSIG_KS_DELis never checked even thoughbr/pkg/gc/manager_keyspace.gowrites that marker on delete.Suggested patch
wait "$global_pid" || { echo "Global backup failed"; exit 1; } wait_file_exists "$SIG_GLOBAL_DEL" "(waiting global delete)" +test ! -f "$SIG_KS_DEL" safe_point=$(run_pd_ctl -u https://$PD_ADDR service-gc-safepoint) if echo "$safe_point" | grep -q "\"service_id\": \"$global_sp_id\""; then🤖 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 `@br/tests/br_gc_keyspace_full_backup/run.sh` around lines 74 - 81, Add an assertion that the keyspace delete marker (SIG_KS_DEL) is absent in global mode: after verifying the global safepoint was removed, check that the SIG_KS_DEL marker file does not exist (similar to how SIG_KS_SET is checked) and fail the script if it is present; reference the existing variables SIG_KS_DEL and SIG_KS_SET and the helper wait_file_exists/run_pd_ctl pattern so the new check mirrors the other marker checks (use wait_file_not_exists or a direct test and exit 1 if SIG_KS_DEL is found).br/pkg/gc/mock_test.go (1)
87-99: ⚡ Quick winSurface fixture teardown failures instead of discarding them.
The shared cleanup helper currently ignores
rm.Close,db.Close, andos.RemoveAllerrors. If fixture shutdown starts failing, these tests will hide the root cause and become much harder to debug.As per coding guidelines, "Go code: Keep error handling actionable and contextual; avoid silently swallowing errors"
🤖 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 `@br/pkg/gc/mock_test.go` around lines 87 - 99, The cleanup helper in t.Cleanup currently swallows errors from rm.Close, db.Close, and os.RemoveAll; change it to check each call's returned error and fail the test with context when non-nil (e.g. if err := rm.Close(); err != nil { t.Fatalf("rm.Close failed: %v", err) }), do the same for db.Close and os.RemoveAll including the dbPath/logPath in the message so teardown failures surface (refer to the t.Cleanup block and variables rm, db, dbPath, logPath).
🤖 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 `@br/pkg/gc/manager_keyspace.go`:
- Around line 14-17: The build fails because github.com/tikv/client-go/v2 pulls
in github.com/tikv/pd/client/circuitbreaker which is missing from the pinned PD
client; to fix, update the module/dependency pinning so the PD client package
version that includes circuitbreaker is used (or backport that package) and
update go.mod and Bazel/go dependency files accordingly; ensure the PD client
you pin still provides GetGCStatesClient, SetGCBarrier, and DeleteGCBarrier
(used by manager_keyspace.go via clients/gc) and re-run go test ./br/pkg/gc to
verify the import resolution.
In `@br/pkg/gc/mock_test.go`:
- Line 1: Replace the one-line header in br/pkg/gc/mock_test.go with the
standard TiDB Go license boilerplate (full copyright + Apache-2.0 block) used
across the repo; apply the same replacement to other new *.go test files so that
files like mock_test.go contain the required multi-line TiDB license header at
the top before the package declaration.
In `@br/pkg/gc/safepoint_test.go`:
- Around line 44-61: The test Concurrent_Uniqueness in TestMakeSafePointID calls
require.False(t, ...) from worker goroutines which is unsafe; change it so
workers only send their generated IDs back (e.g., via a channel or append to a
protected slice/map) and do uniqueness checks and the require.False/assertions
on the main test goroutine after wg.Wait(); update references to
gc.MakeSafePointID (worker) and remove require.False from the anonymous
goroutine so the main goroutine reads all IDs, detects duplicates, and calls
require on the results.
In `@br/pkg/gc/safepoint.go`:
- Around line 58-60: The helpers (e.g., CheckGCSafePoint) currently call
mgr.GetGCSafePoint without validating mgr, causing a panic when mgr is nil;
update CheckGCSafePoint and the other helper(s) in the same file (lines ~73-86)
to check if mgr == nil at the top and immediately return ErrInvalidArgument if
so, avoiding panics and making failures predictable for miswired tests and
callers. Ensure the nil-check precedes any use of mgr (including
mgr.GetGCSafePoint) and return the existing ErrInvalidArgument sentinel value.
In `@br/pkg/restore/log_client/client.go`:
- Around line 1993-2003: The defer uses the original request context (ctx) when
calling gcMgr.DeleteServiceSafePoint, which can be canceled/expired and prevent
safepoint removal; change the teardown to create a fresh bounded background
context (e.g., ctx2 := context.WithTimeout(context.Background(),
<reasonable-duration>) with cancel) and call gcMgr.DeleteServiceSafePoint(ctx2,
sp) inside the defer, then cancel ctx2 after the delete; ensure this new context
and its cancel are used instead of ctx while keeping the existing
gcSafePointKeeperCancel call and referencing cctx, gcSafePointKeeperCancel,
gcMgr.DeleteServiceSafePoint, and sp.
In `@br/pkg/task/common_test.go`:
- Around line 13-18: Resolve the merge conflict markers in the import block by
removing the `<<<<<<<`, `=======`, and `>>>>>>>` lines and restoring a single
import list that includes both the required packages
`github.com/pingcap/tidb/br/pkg/storage` (needed for storage.BackendOptions) and
`github.com/pingcap/tidb/br/pkg/gc` (needed for gc.DefaultBRGCSafePointTTL`),
and remove `github.com/pingcap/tidb/br/pkg/utils` if it remains and is unused in
the file.
In `@br/pkg/task/stream.go`:
- Around line 437-445: streamMgr.setGCSafePoint currently always calls
GetGCManager().SetServiceSafePoint which fails to remove a safepoint when TTL==0
in keyspace mode; update setGCSafePoint to detect sp.TTL == 0 (or the equivalent
sentinel used by RunStreamStop) and call
GetGCManager().DeleteServiceSafePoint(ctx, sp) instead of SetServiceSafePoint
for removals, otherwise keep calling SetServiceSafePoint(ctx, sp); reference the
streamMgr.setGCSafePoint function and GetGCManager().DeleteServiceSafePoint /
SetServiceSafePoint methods when making the change.
In `@br/pkg/utils/BUILD.bazel`:
- Around line 61-65: Remove the unresolved git merge markers and decide which
dependency(s) should remain: either keep
"`@com_github_tikv_client_go_v2//oracle`", keep
"`@com_github_prometheus_client_golang//prometheus/promhttp`", or include both if
required by the package; specifically delete the lines starting with "<<<<<<<",
"=======", and ">>>>>>>" and update the dependency list to contain only the
correct Bazel labels so the BUILD rule is syntactically valid.
In `@tests/realtikvtest/brietest/BUILD.bazel`:
- Around line 19-22: Remove the leftover merge-conflict markers (<<<<<<<,
=======, >>>>>>>) from the BUILD.bazel fragment and ensure the brietest_test
target has the intended attribute shard_count = 27 present; specifically, delete
the conflict markers and keep the line "shard_count = 27," inside the
brietest_test target so Bazel can parse the file cleanly.
In `@tests/realtikvtest/brietest/gc_keyspace_test.go`:
- Around line 45-48: Save the original global config before mutating it (call
config.GetGlobalConfig() into a variable like prevCfg), then after setting
cfg.Store and cfg.Path and calling config.StoreGlobalConfig(cfg) register a
t.Cleanup that restores the original via config.StoreGlobalConfig(prevCfg); use
the same variable names (cfg, prevCfg) and t.Cleanup so the process-wide state
is reverted when the test finishes.
---
Outside diff comments:
In `@br/pkg/gc/safepoint.go`:
- Around line 78-93: The code can panic when sp.TTL is 1 or 2 because
updateGapTime becomes zero and time.NewTicker(0) panics; before creating the
ticker (and before calling mgr.SetServiceSafePoint), validate or clamp sp.TTL to
a safe minimum (e.g., reject TTLs < 3 seconds with an error from the same
validation block that checks sp.ID/TTL, or set sp.TTL = 3 when computing
updateGapTime), then compute updateGapTime = time.Duration(sp.TTL) * time.Second
/ preUpdateServiceSafePointFactor and create updateTick and checkTick; reference
sp.TTL, updateGapTime, preUpdateServiceSafePointFactor, updateTick and the
earlier validation that returns ErrInvalidArgument so the fix is applied in the
same validation/initialization area.
---
Nitpick comments:
In `@br/pkg/conn/conn.go`:
- Around line 279-285: Add a godoc comment for the exported GetGCManager method
on type Mgr to match the existing SetGcManager documentation and keep
exported-symbol docs consistent; write a concise, semantic comment above func
(mgr *Mgr) GetGCManager() gc.Manager that explains what GC manager is returned
or under what conditions (e.g., "GetGCManager returns the current
garbage-collection manager used by the Mgr." or similar) without restating the
method name.
In `@br/pkg/gc/mock_test.go`:
- Around line 87-99: The cleanup helper in t.Cleanup currently swallows errors
from rm.Close, db.Close, and os.RemoveAll; change it to check each call's
returned error and fail the test with context when non-nil (e.g. if err :=
rm.Close(); err != nil { t.Fatalf("rm.Close failed: %v", err) }), do the same
for db.Close and os.RemoveAll including the dbPath/logPath in the message so
teardown failures surface (refer to the t.Cleanup block and variables rm, db,
dbPath, logPath).
In `@br/tests/br_gc_keyspace_full_backup/run.sh`:
- Around line 74-81: Add an assertion that the keyspace delete marker
(SIG_KS_DEL) is absent in global mode: after verifying the global safepoint was
removed, check that the SIG_KS_DEL marker file does not exist (similar to how
SIG_KS_SET is checked) and fail the script if it is present; reference the
existing variables SIG_KS_DEL and SIG_KS_SET and the helper
wait_file_exists/run_pd_ctl pattern so the new check mirrors the other marker
checks (use wait_file_not_exists or a direct test and exit 1 if SIG_KS_DEL is
found).
🪄 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: 57a0aa7c-0068-4df4-acec-4032a83a2fd1
📒 Files selected for processing (31)
br/pkg/backup/BUILD.bazelbr/pkg/backup/client.gobr/pkg/backup/client_test.gobr/pkg/conn/BUILD.bazelbr/pkg/conn/conn.gobr/pkg/gc/BUILD.bazelbr/pkg/gc/manager.gobr/pkg/gc/manager_global.gobr/pkg/gc/manager_keyspace.gobr/pkg/gc/manager_test.gobr/pkg/gc/mock_test.gobr/pkg/gc/safepoint.gobr/pkg/gc/safepoint_test.gobr/pkg/restore/log_client/BUILD.bazelbr/pkg/restore/log_client/client.gobr/pkg/task/BUILD.bazelbr/pkg/task/backup.gobr/pkg/task/backup_ebs.gobr/pkg/task/common_test.gobr/pkg/task/operator/BUILD.bazelbr/pkg/task/operator/prepare_snap.gobr/pkg/task/restore.gobr/pkg/task/restore_data.gobr/pkg/task/stream.gobr/pkg/utils/BUILD.bazelbr/pkg/utils/safe_point_test.gobr/tests/br_gc_keyspace_full_backup/run.shbr/tests/run_group_br_tests.shtests/realtikvtest/brietest/BUILD.bazeltests/realtikvtest/brietest/gc_keyspace_test.gotests/realtikvtest/brietest/operator_test.go
💤 Files with no reviewable changes (1)
- br/pkg/utils/safe_point_test.go
| @@ -0,0 +1,214 @@ | |||
| // Copyright 2025 PingCAP, Inc. Licensed under Apache-2.0. | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the standard TiDB Go license header.
This abbreviated one-line header doesn't match the required TiDB boilerplate for new *.go files. Please replace it here and in the other new Go test files with the standard copyright + Apache 2.0 block.
As per coding guidelines, "**/*.go: For new source files (*.go): include the standard TiDB license header (copyright + Apache 2.0)`"
🤖 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 `@br/pkg/gc/mock_test.go` at line 1, Replace the one-line header in
br/pkg/gc/mock_test.go with the standard TiDB Go license boilerplate (full
copyright + Apache-2.0 block) used across the repo; apply the same replacement
to other new *.go test files so that files like mock_test.go contain the
required multi-line TiDB license header at the top before the package
declaration.
| t.Run("Concurrent_Uniqueness", func(t *testing.T) { | ||
| var mu sync.Mutex | ||
| ids := make(map[string]bool) | ||
| count := 100 | ||
|
|
||
| var wg sync.WaitGroup | ||
| for i := 0; i < count; i++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| id := gc.MakeSafePointID() | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| require.False(t, ids[id], "duplicate ID generated: %s", id) | ||
| ids[id] = true | ||
| }() | ||
| } | ||
| wg.Wait() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does github.com/stretchr/testify allow calling require/assert helpers from goroutines created inside a test?
💡 Result:
No, you should not call testify's require or assert helpers from goroutines created inside a test [1][2][3]. According to the official documentation for github.com/stretchr/testify, these functions must be called from the goroutine running the test or benchmark function itself [1][2][3]. Calling them from other goroutines can result in race conditions [1][2][3]. This restriction exists primarily because testify helpers interact with the standard library's testing.T object [4][5]. Specifically, the require package uses t.FailNow, and the assert package uses t.Errorf [6][4]. The Go testing package documentation mandates that t.FailNow must be called from the goroutine running the test, as it uses runtime.Goexit to terminate that specific goroutine [4][5]. While t.Errorf does not have the same immediate termination behavior, using either from concurrent goroutines is generally discouraged to avoid data races and ensure test results are correctly associated with the main test execution flow [1][2][6][5]. If you need to perform assertions within a goroutine, it is standard practice to use channels or other synchronization mechanisms to collect the results and perform the assertions on the main test goroutine [6].
Citations:
- 1: https://pkg.go.dev/github.com/stretchr/testify/require?utm%5C_source=godoc
- 2: https://pkg.go.dev/github.com/stretchr/testify/require
- 3: https://pkg.go.dev/github.com/stretchr/testify
- 4: https://stackoverflow.com/questions/72850921/go-assertion-utility-functions-behave-like-non-blocking-operation
- 5: https://github.com/stretchr/testify/pull/1392](https:/togithub.com/stretchr/testify/pull/1392)
- 6: How to handle failed expectations inside of a goroutine? stretchr/testify#772
Move require assertions out of the worker goroutines in TestMakeSafePointID/Concurrent_Uniqueness.
In br/pkg/gc/safepoint_test.go (lines 44-61), the goroutines call require.False(t, ...). testify/require uses testing.T (via FailNow) and should only be called from the test/benchmark goroutine; doing it from spawned goroutines can make the test flaky/racy. Collect duplicate IDs from workers (e.g., channel/atomic), then perform the require once after wg.Wait() on the main test goroutine.
🤖 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 `@br/pkg/gc/safepoint_test.go` around lines 44 - 61, The test
Concurrent_Uniqueness in TestMakeSafePointID calls require.False(t, ...) from
worker goroutines which is unsafe; change it so workers only send their
generated IDs back (e.g., via a channel or append to a protected slice/map) and
do uniqueness checks and the require.False/assertions on the main test goroutine
after wg.Wait(); update references to gc.MakeSafePointID (worker) and remove
require.False from the anonymous goroutine so the main goroutine reads all IDs,
detects duplicates, and calls require on the results.
| func CheckGCSafePoint(ctx context.Context, mgr Manager, ts uint64) error { | ||
| safePoint, err := mgr.GetGCSafePoint(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
Fail fast when the GC manager is missing.
These new helpers call through mgr immediately. A nil gc.Manager now panics, and the new test wiring in br/pkg/backup/client_test.go already shows zero-value conn.Mgr instances need manual GC-manager initialization. Returning ErrInvalidArgument here would make miswired tests and alternate callers fail predictably.
Proposed fix
func CheckGCSafePoint(ctx context.Context, mgr Manager, ts uint64) error {
+ if mgr == nil {
+ return errors.Annotate(berrors.ErrInvalidArgument, "gc manager is required")
+ }
safePoint, err := mgr.GetGCSafePoint(ctx)
if err != nil {
log.Warn("fail to get GC safe point", zap.Error(err))
@@
func StartServiceSafePointKeeper(
ctx context.Context,
sp BRServiceSafePoint,
mgr Manager,
) error {
+ if mgr == nil {
+ return errors.Annotate(berrors.ErrInvalidArgument, "gc manager is required")
+ }
if sp.ID == "" || sp.TTL <= 0 {
return errors.Annotatef(berrors.ErrInvalidArgument, "invalid service safe point %v", sp)
}Also applies to: 73-86
🤖 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 `@br/pkg/gc/safepoint.go` around lines 58 - 60, The helpers (e.g.,
CheckGCSafePoint) currently call mgr.GetGCSafePoint without validating mgr,
causing a panic when mgr is nil; update CheckGCSafePoint and the other helper(s)
in the same file (lines ~73-86) to check if mgr == nil at the top and
immediately return ErrInvalidArgument if so, avoiding panics and making failures
predictable for miswired tests and callers. Ensure the nil-check precedes any
use of mgr (including mgr.GetGCSafePoint) and return the existing
ErrInvalidArgument sentinel value.
| cctx, gcSafePointKeeperCancel := context.WithCancel(ctx) | ||
| defer func() { | ||
| log.Info("start to remove gc-safepoint keeper") | ||
| // close the gc safe point keeper at first | ||
| gcSafePointKeeperCancel() | ||
| // set the ttl to 0 to remove the gc-safe-point | ||
| sp.TTL = 0 | ||
| if err := utils.UpdateServiceSafePoint(ctx, pdClient, sp); err != nil { | ||
| log.Warn("failed to update service safe point, backup may fail if gc triggered", | ||
| // remove the gc-safe-point | ||
| if err := gcMgr.DeleteServiceSafePoint(ctx, sp); err != nil { | ||
| log.Warn("failed to remove service safe point, backup may fail if gc triggered", | ||
| zap.Error(err), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Use a fresh cleanup context for safepoint removal.
If this helper returns because ctx is canceled or its deadline expires, gcMgr.DeleteServiceSafePoint(ctx, sp) will fail immediately and leave the service safepoint / keyspace barrier behind until TTL expiry. The teardown should use its own bounded background context instead of the request context.
Suggested fix
defer func() {
log.Info("start to remove gc-safepoint keeper")
// close the gc safe point keeper at first
gcSafePointKeeperCancel()
+ cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
// remove the gc-safe-point
- if err := gcMgr.DeleteServiceSafePoint(ctx, sp); err != nil {
+ if err := gcMgr.DeleteServiceSafePoint(cleanupCtx, sp); err != nil {
log.Warn("failed to remove service safe point, backup may fail if gc triggered",
zap.Error(err),
)
}
log.Info("finish removing gc-safepoint keeper")
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cctx, gcSafePointKeeperCancel := context.WithCancel(ctx) | |
| defer func() { | |
| log.Info("start to remove gc-safepoint keeper") | |
| // close the gc safe point keeper at first | |
| gcSafePointKeeperCancel() | |
| // set the ttl to 0 to remove the gc-safe-point | |
| sp.TTL = 0 | |
| if err := utils.UpdateServiceSafePoint(ctx, pdClient, sp); err != nil { | |
| log.Warn("failed to update service safe point, backup may fail if gc triggered", | |
| // remove the gc-safe-point | |
| if err := gcMgr.DeleteServiceSafePoint(ctx, sp); err != nil { | |
| log.Warn("failed to remove service safe point, backup may fail if gc triggered", | |
| zap.Error(err), | |
| ) | |
| } | |
| defer func() { | |
| log.Info("start to remove gc-safepoint keeper") | |
| // close the gc safe point keeper at first | |
| gcSafePointKeeperCancel() | |
| cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| // remove the gc-safe-point | |
| if err := gcMgr.DeleteServiceSafePoint(cleanupCtx, sp); err != nil { | |
| log.Warn("failed to remove service safe point, backup may fail if gc triggered", | |
| zap.Error(err), | |
| ) | |
| } | |
| log.Info("finish removing gc-safepoint keeper") | |
| }() |
🤖 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 `@br/pkg/restore/log_client/client.go` around lines 1993 - 2003, The defer uses
the original request context (ctx) when calling gcMgr.DeleteServiceSafePoint,
which can be canceled/expired and prevent safepoint removal; change the teardown
to create a fresh bounded background context (e.g., ctx2 :=
context.WithTimeout(context.Background(), <reasonable-duration>) with cancel)
and call gcMgr.DeleteServiceSafePoint(ctx2, sp) inside the defer, then cancel
ctx2 after the delete; ensure this new context and its cancel are used instead
of ctx while keeping the existing gcSafePointKeeperCancel call and referencing
cctx, gcSafePointKeeperCancel, gcMgr.DeleteServiceSafePoint, and sp.
| <<<<<<< HEAD | ||
| "github.com/pingcap/tidb/br/pkg/storage" | ||
| "github.com/pingcap/tidb/br/pkg/utils" | ||
| ======= | ||
| "github.com/pingcap/tidb/br/pkg/gc" | ||
| >>>>>>> cb138ca3deb (br: add keyspace-aware GC safepoint support for backup and restore (#65483)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,30p' br/pkg/task/common_test.go
rg -n '^(<<<<<<<|=======|>>>>>>>)' br/pkg/task/common_test.goRepository: pingcap/tidb
Length of output: 949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="br/pkg/task/common_test.go"
rg -n "storage\.BackendOptions|gc\.DefaultBRGCSafePointTTL" "$file" || true
rg -n "expectedDefaultConfig|expectedDefaultBackupConfig" "$file" || true
sed -n '1,120p' "$file"Repository: pingcap/tidb
Length of output: 4130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="br/pkg/task/common_test.go"
rg -n "utils\." "$file" || trueRepository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="br/pkg/task/common_test.go"
rg -n "br/pkg/utils|\"github.com/pingcap/tidb/br/pkg/utils\"|utils\." "$file" || true
rg -n "br/pkg/gc|\"github.com/pingcap/tidb/br/pkg/gc\"|gc\." "$file" || trueRepository: pingcap/tidb
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="br/pkg/task/common_test.go"
rg -n "\butils\b" "$file" || trueRepository: pingcap/tidb
Length of output: 99
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="br/pkg/task/common_test.go"
echo "---- conflict markers ----"
rg -n '^(<<<<<<<|=======|>>>>>>>)' "$file" || true
echo "---- utils usage ----"
rg -n '\butils\.' "$file" || true
echo "---- storage usage ----"
rg -n '\bstorage\.' "$file" || true
echo "---- gc usage ----"
rg -n '\bgc\.' "$file" || true
echo "---- import block (around lines 1-60) ----"
sed -n '1,70p' "$file"Repository: pingcap/tidb
Length of output: 2211
Resolve merge-conflict markers in br/pkg/task/common_test.go import block — the unresolved <<<<<<</=======/>>>>>>> markers keep this test file from compiling. After resolving, keep both br/pkg/storage (used via storage.BackendOptions) and br/pkg/gc (used via gc.DefaultBRGCSafePointTTL); br/pkg/utils is not referenced in this file and should be removed if it remains unused.
Suggested fix
-<<<<<<< HEAD
- "github.com/pingcap/tidb/br/pkg/storage"
- "github.com/pingcap/tidb/br/pkg/utils"
-=======
- "github.com/pingcap/tidb/br/pkg/gc"
->>>>>>> cb138ca3deb (br: add keyspace-aware GC safepoint support for backup and restore (`#65483`))
+ "github.com/pingcap/tidb/br/pkg/gc"
+ "github.com/pingcap/tidb/br/pkg/storage"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| "github.com/pingcap/tidb/br/pkg/storage" | |
| "github.com/pingcap/tidb/br/pkg/utils" | |
| ======= | |
| "github.com/pingcap/tidb/br/pkg/gc" | |
| >>>>>>> cb138ca3deb (br: add keyspace-aware GC safepoint support for backup and restore (#65483)) | |
| "github.com/pingcap/tidb/br/pkg/gc" | |
| "github.com/pingcap/tidb/br/pkg/storage" |
🧰 Tools
🪛 GitHub Check: Compile for FreeBSD job
[failure] 13-13:
missing import path
🪛 GitHub Check: Compile for macos-latest
[failure] 13-13:
missing import path
🪛 GitHub Check: Compile for ubuntu-latest
[failure] 13-13:
missing import path
🤖 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 `@br/pkg/task/common_test.go` around lines 13 - 18, Resolve the merge conflict
markers in the import block by removing the `<<<<<<<`, `=======`, and `>>>>>>>`
lines and restoring a single import list that includes both the required
packages `github.com/pingcap/tidb/br/pkg/storage` (needed for
storage.BackendOptions) and `github.com/pingcap/tidb/br/pkg/gc` (needed for
gc.DefaultBRGCSafePointTTL`), and remove `github.com/pingcap/tidb/br/pkg/utils`
if it remains and is unused in the file.
| func (s *streamMgr) setGCSafePoint(ctx context.Context, sp gc.BRServiceSafePoint) error { | ||
| err := gc.CheckGCSafePoint(ctx, s.mgr.GetGCManager(), sp.BackupTS) | ||
| if err != nil { | ||
| return errors.Annotatef(err, | ||
| "failed to check gc safePoint, ts %v", sp.BackupTS) | ||
| } | ||
|
|
||
| err = utils.UpdateServiceSafePoint(ctx, s.mgr.GetPDClient(), sp) | ||
| err = s.mgr.GetGCManager().SetServiceSafePoint(ctx, sp) | ||
| if err != nil { |
There was a problem hiding this comment.
Use DeleteServiceSafePoint for TTL=0 removals.
This helper now always routes through SetServiceSafePoint, but RunStreamStop still uses TTL: 0 to remove the pause safepoint. That only matches the old global PD API. In keyspace mode the new manager has a separate delete path, so stop can leave the barrier behind instead of clearing it.
Suggested fix
func (s *streamMgr) setGCSafePoint(ctx context.Context, sp gc.BRServiceSafePoint) error {
+ if sp.TTL <= 0 {
+ if err := s.mgr.GetGCManager().DeleteServiceSafePoint(ctx, sp); err != nil {
+ return errors.Trace(err)
+ }
+ log.Info("removed stream safePoint", zap.Object("safePoint", sp))
+ return nil
+ }
+
err := gc.CheckGCSafePoint(ctx, s.mgr.GetGCManager(), sp.BackupTS)
if err != nil {
return errors.Annotatef(err,
"failed to check gc safePoint, ts %v", sp.BackupTS)
}🤖 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 `@br/pkg/task/stream.go` around lines 437 - 445, streamMgr.setGCSafePoint
currently always calls GetGCManager().SetServiceSafePoint which fails to remove
a safepoint when TTL==0 in keyspace mode; update setGCSafePoint to detect sp.TTL
== 0 (or the equivalent sentinel used by RunStreamStop) and call
GetGCManager().DeleteServiceSafePoint(ctx, sp) instead of SetServiceSafePoint
for removals, otherwise keep calling SetServiceSafePoint(ctx, sp); reference the
streamMgr.setGCSafePoint function and GetGCManager().DeleteServiceSafePoint /
SetServiceSafePoint methods when making the change.
| <<<<<<< HEAD | ||
| "@com_github_tikv_client_go_v2//oracle", | ||
| ======= | ||
| "@com_github_prometheus_client_golang//prometheus/promhttp", | ||
| >>>>>>> cb138ca3deb (br: add keyspace-aware GC safepoint support for backup and restore (#65483)) |
There was a problem hiding this comment.
Resolve merge conflict markers before merge.
The unresolved merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>) will break the Bazel build. You must choose between @com_github_tikv_client_go_v2//oracle and @com_github_prometheus_client_golang//prometheus/promhttp, or include both if both are needed by the remaining source files in this package.
🤖 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 `@br/pkg/utils/BUILD.bazel` around lines 61 - 65, Remove the unresolved git
merge markers and decide which dependency(s) should remain: either keep
"`@com_github_tikv_client_go_v2//oracle`", keep
"`@com_github_prometheus_client_golang//prometheus/promhttp`", or include both if
required by the package; specifically delete the lines starting with "<<<<<<<",
"=======", and ">>>>>>>" and update the dependency list to contain only the
correct Bazel labels so the BUILD rule is syntactically valid.
| <<<<<<< HEAD | ||
| ======= | ||
| shard_count = 27, | ||
| >>>>>>> cb138ca3deb (br: add keyspace-aware GC safepoint support for backup and restore (#65483)) |
There was a problem hiding this comment.
Resolve the leftover merge-conflict markers before merge.
Bazel will not parse this file while <<<<<<<, =======, and >>>>>>> are still present, so the brietest_test target is currently broken.
Suggested patch
-<<<<<<< HEAD
-=======
shard_count = 27,
->>>>>>> cb138ca3deb (br: add keyspace-aware GC safepoint support for backup and restore (`#65483`))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| ======= | |
| shard_count = 27, | |
| >>>>>>> cb138ca3deb (br: add keyspace-aware GC safepoint support for backup and restore (#65483)) | |
| shard_count = 27, |
🤖 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 `@tests/realtikvtest/brietest/BUILD.bazel` around lines 19 - 22, Remove the
leftover merge-conflict markers (<<<<<<<, =======, >>>>>>>) from the BUILD.bazel
fragment and ensure the brietest_test target has the intended attribute
shard_count = 27 present; specifically, delete the conflict markers and keep the
line "shard_count = 27," inside the brietest_test target so Bazel can parse the
file cleanly.
| cfg := config.GetGlobalConfig() | ||
| cfg.Store = config.StoreTypeTiKV | ||
| cfg.Path = realtikvtest.PDAddr | ||
| config.StoreGlobalConfig(cfg) |
There was a problem hiding this comment.
Restore the previous global config in t.Cleanup.
config.StoreGlobalConfig mutates process-wide state. Leaving Store and Path pinned to this test's real-TiKV settings can bleed into later brietest cases and make the suite order-dependent.
Suggested patch
- cfg := config.GetGlobalConfig()
- cfg.Store = config.StoreTypeTiKV
- cfg.Path = realtikvtest.PDAddr
- config.StoreGlobalConfig(cfg)
+ prevCfg := *config.GetGlobalConfig()
+ cfg := prevCfg
+ cfg.Store = config.StoreTypeTiKV
+ cfg.Path = realtikvtest.PDAddr
+ config.StoreGlobalConfig(&cfg)
+ t.Cleanup(func() {
+ config.StoreGlobalConfig(&prevCfg)
+ })As per coding guidelines, "Test files: Prefer extending existing test suites and fixtures over creating new scaffolding; keep test changes minimal and deterministic".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg := config.GetGlobalConfig() | |
| cfg.Store = config.StoreTypeTiKV | |
| cfg.Path = realtikvtest.PDAddr | |
| config.StoreGlobalConfig(cfg) | |
| prevCfg := *config.GetGlobalConfig() | |
| cfg := prevCfg | |
| cfg.Store = config.StoreTypeTiKV | |
| cfg.Path = realtikvtest.PDAddr | |
| config.StoreGlobalConfig(&cfg) | |
| t.Cleanup(func() { | |
| config.StoreGlobalConfig(&prevCfg) | |
| }) |
🤖 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 `@tests/realtikvtest/brietest/gc_keyspace_test.go` around lines 45 - 48, Save
the original global config before mutating it (call config.GetGlobalConfig()
into a variable like prevCfg), then after setting cfg.Store and cfg.Path and
calling config.StoreGlobalConfig(cfg) register a t.Cleanup that restores the
original via config.StoreGlobalConfig(prevCfg); use the same variable names
(cfg, prevCfg) and t.Cleanup so the process-wide state is reverted when the test
finishes.
|
@ti-chi-bot: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
ti-chi-bot
Bot
added
cherry-pick-approved
# Conflicts: # br/pkg/task/common_test.go # br/tests/run_group_br_tests.sh
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
|
/unhold |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68863 +/- ##
================================================
Coverage ? 55.6514%
================================================
Files ? 1854
Lines ? 666894
Branches ? 0
================================================
Hits ? 371136
Misses ? 268102
Partials ? 27656
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/test unit-test |
|
@ti-chi-bot: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
ti-chi-bot
Bot
removed
the
cherry-pick-approved
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
4 participants
This is an automated cherry-pick of #65483
What problem does this PR solve?
Issue Number: close #65482
Problem Summary:
BR currently uses the deprecated global GC safepoint API (
pd.Client.UpdateServiceGCSafePoint) which doesn't support keyspace isolation. When users specify--keyspace-nameparameter, BR should use the new per-keyspace GC barrier API (GCStatesClient.SetGCBarrier/DeleteGCBarrier) instead.What changed and how does it work?
This PR introduces a GC manager abstraction layer that automatically selects the appropriate GC implementation based on user's
--keyspace-nameparameter:New files:
gc_manager.go: Factory function and wrapper functionsgc_manager_unified.go: Non-keyspace implementation using deprecated global API (backward compatible)gc_manager_keyspace.go: Keyspace implementation using new GC barrier APIDesign:
Updated callers:
backup.go: UseStartServiceSafePointKeeperWithStoragerestore.go: UseUpdateServiceSafePointWithStoragestream.go: UseStartServiceSafePointKeeperWithStoragebackup_ebs.go,restore_data.go,log_client/client.go: Updated accordinglyCheck List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Release Notes
New Features
Improvements